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.

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

SmallRye Reactive Messaging - Kafka depends on Hibernate Reactive but is built before it #31446

Closed
Sanne opened this issue Feb 27, 2023 · 9 comments · Fixed by #36070
Closed
Assignees
Milestone

Comments

@Sanne
Copy link
Member

Sanne commented Feb 27, 2023

Describe the bug

The build order is currently defined by Maven Reactor as:

  1. Quarkus - Hibernate Reactive - Runtime
  2. Quarkus - SmallRye Reactive Messaging - Kafka - Runtime
  3. Quarkus - SmallRye Reactive Messaging - Kafka - Deployment
  4. Quarkus - Hibernate Reactive - Deployment

This results in tricky problems during the maintenance and development of Quarkus; not sure if it also affects releases.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

Current main at 1d45952

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.7

Additional information

No response

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 27, 2023

/cc @DavideD (hibernate-reactive), @Ladicek (smallrye), @alesj (kafka), @cescoffier (kafka,reactive-messaging), @gavinking (hibernate-reactive), @jmartisk (smallrye), @ozangunalp (kafka,reactive-messaging), @phillip-kruger (smallrye), @radcortez (smallrye)

@ozangunalp
Copy link
Contributor

@Sanne SmallRye Reactive Messaging - Kafka runtime has optional dependencies for redis-client, hibernate-reactive and hibernate-orm runtime modules.

But the SmallRye Reactive Messaging - Deployment module doesn't have any of those dependencies.
The reactor order seems correct to me.

@Sanne
Copy link
Member Author

Sanne commented Feb 27, 2023

Are you sure the hibernate-reactive extension doesn't get activated during integration tests?

I had a failure earlier which clearly was triggered by me making changes to both the Hibernate Reactive Runtime and Deployment modules; so the order looks suspicious to me as Maven recognized the need to rebuild the HR runtime module first, but not the matching deployment.

Remember that in Quarkus the deployment modules get pulled in automatically to match their runtime counterparts, but Maven doesn't know about it. This is handled by our customizations, therefore it's not automatic but we need to handle the order with care, and sometimes add an artificial dependency to ensure correct order of build is defined.

Unfortunately since I'm working with snapshtos it's hard to try reproduce the earlier error, but the order looks wrong to me - it would certainly be safer to ensure that Runtime and Deployment modules of each extension are not build "interleaved".

You need something like this:

https://github.com/quarkusio/quarkus/blob/main/integration-tests/hibernate-orm-envers/pom.xml#L49-L75

Alternatively, get rid of the optional dependency and move that into a dedicated module which provides the bridge code and depends on both. Optional dependency are a pain to maintain ;)

@ozangunalp
Copy link
Contributor

I knew about the deployment modules getting pulled in, but didn't imagine it would cause such an issue, especially since the deployment modules are not really dependent. Did you have that using --also-make ?

There are IT modules testing hibernate-orm with smallrye-reactive-kafka and hibernate-reactive with smallrye-reactive-kafka. But the -deployment module dependencies are there for those IT modules.

I know optional dependencies are hard. But sometimes it is practical :) At least they are in the same repo.

@Sanne
Copy link
Member Author

Sanne commented Feb 28, 2023

I knew about the deployment modules getting pulled in, but didn't imagine it would cause such an issue, especially since the deployment modules are not really dependent. Did you have that using --also-make ?

Right, when making changes to some module I run the tests targeting the patched modules with -amd , so to have it rebuild my modules and anything dependent on it.
But I need to be able to trust the Maven reactor order to build things in the right sequence, and not least to identify which modules need to be rebuilt at all. Unfortunately the way our build works this is all quite fragile.

I know optional dependencies are hard. But sometimes it is practical :) At least they are in the same repo.

Ok, as you prefer :)

@ozangunalp
Copy link
Contributor

Ok, I think I have a lead for the problem. Those integration tests use panache dependencies and so adds its ...panache-deployment dependency with * exclusions. But that won't pull quarkus-hibernate-reactive-deployment or quarkus-hibernate-orm-deployment.

@ozangunalp
Copy link
Contributor

The easiest way I can think of to force the build order is to add a test dependency from reactive-messaging-kafka-deployment to other deployment modules.

@cescoffier
Copy link
Member

@ozangunalp any update on this one?

@ozangunalp
Copy link
Contributor

Sorry I forgot about this. Did some tests and opened #36070

@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 25, 2023
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this issue Sep 25, 2023
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this issue Sep 25, 2023
michelle-purcell pushed a commit to michelle-purcell/quarkus that referenced this issue Sep 25, 2023
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants