Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make reactive-oracle-client depend on jdbc-oracle #25624

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Make reactive-oracle-client depend on jdbc-oracle #25624

merged 3 commits into from
Jun 8, 2022

Conversation

tsegismont
Copy link
Contributor

Fixes #25415

Reactive Oracle Client extension does not work in native mode.
Instead of duplicating code, this PR makes the reactive client extension depend on jdbc client extension.

Also, it enables Reactive Oracle Client integration tests in native mode.

@tsegismont tsegismont requested a review from cescoffier May 17, 2022 12:54
@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label May 17, 2022
@tsegismont
Copy link
Contributor Author

cc @rsvoboda @Sanne

@tsegismont tsegismont mentioned this pull request May 17, 2022
Closed
cescoffier
cescoffier approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's see what the CI says.

@tsegismont
Copy link
Contributor Author

The test failures looks unrelated:

[INFO] Running io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest
Error:  Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.01 s <<< FAILURE! - in io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest
Error:  io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest.testRegistrySnapshotPlatformCatalog  Time elapsed: 0 s  <<< ERROR!
java.lang.IllegalStateException: Failed to locate the dev tools config file (.quarkus/config.yaml)

@quarkus-bot

This comment has been minimized.

Sign in to view
Sanne
Sanne requested changes May 18, 2022
View reviewed changes
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's wise to pull in the JDBC quarkus extension as a dependency as that will trigger transitive dependencies which are necessary to setup a JDBC pool, and trigger other build items related to JDBC use of Oracle - which we likely want to skip in this case.

Could you extract a common module for the stuff you need to reuse? Or actually I'm not even sure if there's much in common, you most likely want to reuse some of the native-image support rules but they are most likely going to be needing different tuning.

Sanne
Sanne requested changes May 18, 2022
View reviewed changes
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to suggest making some changes.

tsegismont added 2 commits June 3, 2022 09:33
@tsegismont
Make reactive-oracle-client depend on jdbc-oracle
08d5988 
Fixes quarkusio#25415

Reactive Oracle Client extension does not work in native mode.
Instead of duplicating code, this commit makes the reactive client extension depend on jdbc client extension.
@tsegismont
Enable Reactive Oracle Client integration tests in native mode
bbfc229
@quarkus-bot

This comment has been minimized.

Sign in to view
@tsegismont
Fix failure to resolve implicit db kind
56ebc43 
When there are multiple candidates for the same kind (i.e. reactive + jdbc), an implicit db kind can be chosen instead of returning an empty optional.
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 3, 2022

Failing Jobs - Building 56ebc43

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/vertx/deployment 
! Skipped: extensions/agroal/deployment extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 334 more

📦 extensions/vertx/deployment

✖ io.quarkus.vertx.DuplicatedContextTest.testThatBlockingEventConsumersAreCalledOnDuplicatedContext - More details - Source on GitHub

(RECIPIENT_FAILURE,8185) io.smallrye.mutiny.TimeoutException
	at io.vertx.core.eventbus.Message.fail(Message.java:141)
	at io.quarkus.vertx.runtime.VertxRecorder$3$1$1.handle(VertxRecorder.java:117)

@tsegismont
Copy link
Contributor Author

@Sanne @cescoffier since we've come to an agreement about this PR, can we go ahead and merge it? The single CI failure is unrelated.

@tsegismont tsegismont requested a review from Sanne June 8, 2022 12:45
Sanne
Sanne approved these changes Jun 8, 2022
View reviewed changes
Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - sorry for the delay, have been travelling.

@Sanne Sanne merged commit 8aef746 into quarkusio:main Jun 8, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone Jun 8, 2022
@Sanne
Copy link
Member

Sanne commented Jun 8, 2022

thanks @tsegismont !

@Sanne Sanne added area/reactive-sql-clients and removed area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure labels Jun 8, 2022
@tsegismont tsegismont deleted the issue_25415 branch July 4, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sanne Sanne Sanne approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/reactive-sql-clients kind/bugfix
Projects
None yet
Milestone
2.10.0.CR1
Development

Successfully merging this pull request may close these issues.

integration-tests/reactive-oracle-client fails in native mode
3 participants
@tsegismont @Sanne @cescoffier