-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Added more relevant constraints to the quarkus-bom #26565
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yrodiere could you have a look at this when you're back? It looks like we cannot enforce Hibernate Search's version properly when using Infinispan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this comment specifically: #26565 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet wdyt about merging it a figuring out the hibernate-search/infinispan compatibility separately? Given that the rest of the constraints aren't related to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsmet I can have a look but I'm not sure what I can do? The version of Infinispan bundled with Quarkus hasn't been updated to HSearch 6.1 yet. Infinispan's
main
branch has been upgraded, but I don't think it's been published yet.The only immediate solutions I see are:
In the future, there are a few possible strategies, none of them great:
Personally I'd favor 4. It's really supposed to be an internal dependency of Infinispan after all, since they're working hard on hiding all Hibernate Search APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time I checked there wasn't a more recent Infinispan version for us to upgrade to, unfortunately.
Never breaking the SPI (2) is a great one :) And if you do, a safe way would be to coordinate releases, so that there is combination of Hibernate Search and Infinispan that works, which is (3).
Shading is an option, although it may complicate CVE tracking and patch delivery. But maybe not that much.
As to how wise downgrading the HS version is, it depends on how often people will run into this issue, basically how critical it is to have a working combination of the two. I am not sure for long it has been broken and whether anybody has run into it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite all the complications, we shouldn't just let it go neither. Today we could document it as not being supported. Until one day we get one or more important use-cases that require both HS and Infinispan being present in the same app, at that moment we'll have to make it work somehow and starting from that moment, we will want to preserve compatibility and not have random breakages between releases. So thinking/preparing for that moment could be worth it.
@gsmet I don't think the rest of the changes should be blocked by this issue. Would you prefer to remove the comments I added about the HS/Infinispan issue from the BOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I will merge it, I was busy on other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not random, it's when necessary, on minor versions, in SPIs only, and that's a clearly established policy for all Hibernate projects. Let's not assume we're breaking stuff just because we don't care: we do :)
The exact reason we're having a problem is I'd been told in the past that there would not be any Infinispan Embedded in Quarkus, and since the HotRod (non-Embedded) client does not depend on Hibernate Search, the Hibernate Search dependency shouldn't have affected it at all.
Certainly.
I'm back from PTO, so I'll be able to look further. First step would be to determine why the Hibernate Search dependency affects Infnispan at all; I thought Infinispan Embedded had been added to Quarkus without my knowledge, but that doesn't seem to be the case? It seems the error is really just in the HotRod client, which shouldn't be depending on Hibernate Search in the first place. I'll look into that.
What should I do to reproduce the issue; just uncomment those lines, build the relevant modules and run Quarkus + Infinispan integration tests?
Sure, documenting would be a good temporary solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to reproduce uncomment the constraints and run
io.quarkus.it.infinispan.client.HealthCheckTest.testHealthCheck
, you can see the failures from the CI above. Thanks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
For future reference, the problem was in the test dependencies rather than in the "main" dependencies. Guillaume addressed it here: #26647