-
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
Update MS SQL JDBC driver to 11.2.0.jre11 #24010
Update MS SQL JDBC driver to 11.2.0.jre11 #24010
Conversation
I ran the MS SQL integration test locally without any problem. |
@sschu The PR comment must be changed to Also, make sure the commit message is more readable, something like:
|
I'll be surprised if this works in native w/o further changes, but I'll be happy to be proven wrong.. let's see what CI says about it. |
I wonder if #23933 may help |
e710d75
to
e6d1b97
Compare
Fixed comments, thanks. |
Hi @Sanne! Any reason why this is not covered by dependabot? The other drivers are... |
IIRC, the reason we haven't upgraded yet is that making the new driver work in native is not trivial and require some involved work. But we will see what CI has to say.
Yes, because Dependabot upgrades to the jre17 flavor. And I haven't found a way to properly filter things. |
Ugh...I see. People really need to stop using "classifiers" in versions. |
Apparently this change does not work when building to Native. Some juggling with the GraalVM substitutions may be required |
Unfortunately. :( I only tested the MS SQL integration test and that went fine. I probably lack the skills to fix things with the native build. However, I will give #23933 a try... |
This comment has been minimized.
This comment has been minimized.
Why would it not be necessary anymore? In any case, it did not help. Running the native integration tests I get: |
Because the class/method the substitution is referring to no longer exists in this update |
@gastaldi I played with this a little bit and by adapting substitutions to the new classes, I could make some progress. I also had to add more dependencies like azure-core. Another option I was presented is to provide a flag for classes to load dynamically. I am just missing a guidance on when should I actually add substitution to not trigger more code loading, dynamic loading or actually adding more dependencies... |
Yeah, it's not a trivial task. In my experience, I usually achieve that by trial and error attempts. |
@sschu maybe rebase what you have and share it? Even if not finished, somebody might have a look and provide useful insights. Thanks! |
I am a bit busy atm, will do on Friday... |
e6d1b97
to
5aaa643
Compare
So I played around with this a little bit. I had an intermediate state where things only worked if I add azure-core as dependency. But then things got messy with netty dependencies being added and a conflict where it was declared as initialize-at-runtime in one place and initialize-at-buildtime at another. Surprisingly, it now seems to work also without azure-core- at least the native image build. However, the integration test itself does not succeed anymore - but it also doesn't work for me in main. Its getting a connection reset during the integration test. Not sure where this comes from. @gsmet @gastaldi Maybe you can give it a try if the integration test works for you? |
c0fe94b
to
912bfb4
Compare
I am not 100% sure if Quarkus uses the native-image.properties, I think this feature was disabled some time ago |
We don't generally disable In particular if you enable all features of a library and hard code this, then it becomes much bigger, less optimised code, and also amplifies the compilation times. So that's why in Quarkus we prefer to have "knobs" (the build items) so that we can enable/disable the particular features a developer is needing. But it makes sense for some essential features to be enabled directly in the We're actually having conversations with the GraalVM engineering team about this very topic, wondering how we can help libraries but also without the performance hit; hopefully we'll be able to figure out some way to provide a vendor neutral / standardized approach similar to what Quarkus does, so that metadata can be included within the libraries without making them less "light" when features are not used. |
@Sanne Got it. However, that means everybody trying to build a native image would have to come up with the same list of classes to be initialized at runtime I used for the mssql integration test? Worse even, his list could look a bit different if it uses different code-paths? And therefore using native-image properties could at least provide some baseline? |
912bfb4
to
60ab59b
Compare
60ab59b
to
8f92b01
Compare
This comment has been minimized.
This comment has been minimized.
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.
Seems to work - excellent, thanks!
Fixes quarkusio#24009 Co-authored-by: George Gastaldi <[email protected]>
8f92b01
to
301654e
Compare
rebased the PR and removed the no longer applicable comment about adal4j from the pom.xml |
Thanks a lot! |
Failing Jobs - Building 301654e
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/rest-client integration-tests/smallrye-context-propagation
📦 integration-tests/rest-client✖
✖
✖
✖
📦 integration-tests/smallrye-context-propagation✖
⚙️ JVM Tests - JDK 11 Windows #- Failing: integration-tests/rest-client
📦 integration-tests/rest-client✖
✖
✖
✖
⚙️ JVM Tests - JDK 17 #- Failing: extensions/smallrye-reactive-messaging-kafka/deployment integration-tests/rest-client
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 3 more 📦 extensions/smallrye-reactive-messaging-kafka/deployment✖
📦 integration-tests/rest-client✖
✖
✖
✖
⚙️ JVM Tests - JDK 18 #- Failing: extensions/smallrye-reactive-messaging-kafka/deployment integration-tests/rest-client
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 3 more 📦 extensions/smallrye-reactive-messaging-kafka/deployment✖
📦 integration-tests/rest-client✖
✖
✖
✖
⚙️ Native Tests - HTTP #- Failing: integration-tests/rest-client
📦 integration-tests/rest-client✖
✖
✖
✖
|
Looks like CI is in trouble, but these failing tests are unrelated. I'll merge. |
Related: hibernate/hibernate-orm#5209 |
Fixes #24009