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

HSEARCH-5155 Build with JDK 21 #4171

Merged
merged 6 commits into from
Jul 3, 2024
Merged

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere requested a review from marko-bekhta May 22, 2024 07:02
@marko-bekhta
Copy link
Member

Looks nice 😃 thanks!
Version in GH actions should probably be updated too 🙈

@yrodiere
Copy link
Member Author

Version in GH actions should probably be updated too 🙈

Right, sorry. Done.

@yrodiere
Copy link
Member Author

So... turns out this won't work because impsort maven plugin needs a release... see revelc/impsort-maven-plugin#83.
Moving to draft.

@yrodiere yrodiere marked this pull request as draft May 22, 2024 07:16
@yrodiere yrodiere force-pushed the HSEARCH-5155 branch 2 times, most recently from 5efb7b9 to 11857b3 Compare May 22, 2024 07:52
pom.xml Outdated Show resolved Hide resolved
@yrodiere yrodiere marked this pull request as ready for review May 29, 2024 07:52
@yrodiere
Copy link
Member Author

Draft again... revelc/formatter-maven-plugin#881

@yrodiere yrodiere marked this pull request as draft May 29, 2024 11:19
@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Jun 3, 2024

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@yrodiere
Copy link
Member Author

yrodiere commented Jun 3, 2024

I added an upgrade to formatter-maven-plugin 2.24.0, which supposedly solves the Java 21 compatibility problem... but then it has other problems, so we'll probably have to wait for 2.24.1 or 2.25.0 :(

@yrodiere yrodiere force-pushed the HSEARCH-5155 branch 2 times, most recently from 4eaf74b to 7f7f619 Compare June 5, 2024 06:11
@yrodiere
Copy link
Member Author

yrodiere commented Jun 5, 2024

I added an upgrade to formatter-maven-plugin 2.24.0, which supposedly solves the Java 21 compatibility problem... but then it has other problems, so we'll probably have to wait for 2.24.1 or 2.25.0 :(

Alright, 2.24.1 is out, I updated the PR... and...

[ERROR] Failed to execute goal net.revelc.code:impsort-maven-plugin:1.10.0:sort (import-sorting) on project hibernate-search-parent-integrationtest: Execution import-sorting of goal net.revelc.code:impsort-maven-plugin:1.10.0:sort failed: No enum constant com.github.javaparser.ParserConfiguration.LanguageLevel.JAVA_21 -> [Help 1]

🤦

revelc/impsort-maven-plugin#101 (comment)

😢

@yrodiere
Copy link
Member Author

Added an upgrade to a version of impsort that supports JDK 21, and rebased.

🤞

@yrodiere yrodiere marked this pull request as ready for review July 1, 2024 10:01
….0 to 1.11.0

This adds JDK 21 support in particular.
yrodiere added 3 commits July 1, 2024 13:11
1. Shutdown the thread pool eventually
2. Handle input streams in try-with-resources to please Eclipse
   compiler.
Meaning use JDK 21 for building main artifacts; the baseline (JDK 11) will not change.
@marko-bekhta
Copy link
Member

marko-bekhta commented Jul 1, 2024

😖 😕 there seems to be some precision loss with JDK 21 switch... RangeAggregationSpecificsIT fails in this PR (but only on OpenSearch) as the upper bound on JDK 17 1.58451456E9 and on JDK 21 1.5845146E9 (56 gets rounded to just 6)

@yrodiere
Copy link
Member Author

yrodiere commented Jul 2, 2024

This is very odd... If this is related to the jdk, I'd expect this to affect Elasticsearch as well... ? Or maybe there's some opensearch-specific testing or JSON handling at play?

Did you pinpoint where the precision loss occurs exactly? Compilation of a constant, or Jason parsing/formatting, or... ?

@marko-bekhta
Copy link
Member

yeah ...
looking into this right now 🙈 that info I posted yesterday was by looking at the generated query string. I'll post here once I find something.

@marko-bekhta
Copy link
Member

https://bugs.openjdk.org/browse/JDK-8202555
https://bugs.openjdk.org/browse/JDK-8291475

^ looks like that's the reason 😕 I've tested with JDK 17 (ok) 18 (ok) 19 (not ok) and 21 (not ok) 😄
This "loss" happens inside of the JsonPrimitive when the query is sent.
OpenSearch has a JDK 21 bundled while Elasticsearch has JDK 22. I've tried starting OpenSearch with JDK 22 just to see if that would make a difference, but no.
I also tried running this test on OpenSearch 2.15 - 2.1 and got the same result on all.

I guess let's just update the test value and move on? ( 1584514514.000000184f -> 1584514414.000000184f )

@yrodiere
Copy link
Member Author

yrodiere commented Jul 3, 2024

I guess let's just update the test value and move on? ( 1584514514.000000184f -> 1584514414.000000184f )

+1 if it's a jdk bug, and it's already been reported, not much we can do.

- so that there are no "rounding" problems when converting to string (https://bugs.openjdk.org/browse/JDK-8291475)
Copy link

sonarqubecloud bot commented Jul 3, 2024

@marko-bekhta
Copy link
Member

The build passed! thanks 😃!

@marko-bekhta marko-bekhta merged commit 15be25b into hibernate:main Jul 3, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants