-
Notifications
You must be signed in to change notification settings - Fork 6
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
Ugrade to Quarkus 3.17.5 and Java SDK 3.7.7 #123
Conversation
---------- Upgrade the extension to the latest Quarkus LTS and latest Java SDK release. ------- - Bump Quarkus to 3.15.1 - Bump Java SDK to 3.7.3 - Update netty substitutions. As the official netty extension targets Netty 4.1.111.Final and our SDK uses 4.1.112.Final, some substitutions have been added to the "delete" step of the shell script. Specifically, this method signature has changed between the two versions (4.1.112 has 1 more string endpointIdentificationAlgorithm in the arguments) https://github.com/netty/netty/blob/ebe2aa5b7cd36562a20b024d78ecff47a86874b8/handler/src/main/java/io/netty/handler/ssl/JdkSslClientContext.java#L267-L273 - The legacy @configroot approach for CouchbaseConfig was changed to the new @ConfigMapping one. - Added the compiler arg "-AlegacyConfigRoot=true" to the deployment module pom.xml, as the netty substitution still uses it. - Updated the way CouchbaseDevService gets the config fields.
…ault in DefaultEventBus, register classes for reflection.
…dLoggingTracer, register classes for reflection.
…scovered in the global config. Co-authored-by: David Nault <[email protected]>
...oyment/src/main/java/com/couchbase/quarkus/extension/deployment/CouchbaseDevUiProcessor.java
Outdated
Show resolved
Hide resolved
deployment/src/main/java/com/couchbase/quarkus/extension/deployment/CouchbaseProcessor.java
Outdated
Show resolved
Hide resolved
|
||
@BuildStep | ||
ReflectiveClassBuildItem reflection() { | ||
return ReflectiveClassBuildItem.builder( |
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.
I'm surprised/confused at many of these things needing reflective access. Like CoreConflictResolutionType - it doesn't seem to be used in any reflective way, anywhere in the code base.
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.
They're not used in the extension if that's what you mean, but these are all used in a normal functioning app with the Java SDK.
Quarkus apps can add annotations i.e. @RegisterForReflection
to their own code, but not to third-party libraries like ours so we need to provide it ourselves.
If we don't register them for reflection here, an app built with the extension will fail to serialize/deserialize these classes as it didn't store the reflection info on their fields or methods.
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.
Please forgive my ignorance. Why does a class like CoreConflictResolutionType
need to be serialized/deserialized?
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.
For this one it's a field in CoreBucketSettingsJson
, which is created in ClassicCoreBucketManager.getAllBuckets()
for example. That gives the bucket from the http response as a JsonNode
to the ObjectMapper
: Mapper.convertValue(node, CoreBucketSettingsJson.class);
.
In the native image, (in my understanding) the information about the class and its fields/methods which would usually be retrieved via reflection isn't saved (or maybe stripped out), so the ObjectMapper would not be able to match the classes' field names or types to construct the object.
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.
So we'll need to update this list every time we do Jackson data binding with a new class? :-/
How does this work with Jackson in general? Like, is there a Quarkus extension for Jackson that knows to preserve reflection info for any type used in an objectMapper.convertValue(Object, Class)
statement? Maybe we could use a similar technique to mark Mapper.convertValue
?
In the shorter term, would it be simple to preserve reflection info for all classes in a package? We'd still need to maintain the list of packages, but perhaps updates would be less frequent.
(I'm not suggesting this issue should block the merge; just trying to identify areas that could be improved in the future.)
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.
So we'll need to update this list every time we do Jackson data binding with a new class?
Yes indeed. This list was built by running FIT, and seeing when an reflection error would happen and which operation it affected.
Like, is there a Quarkus extension for Jackson that knows to preserve reflection info for any type used in an objectMapper.convertValue(Object, Class) statement?
For Jackson data-binding this ReflectiveClassBuildItem()
from Quarkus generates a reflection-config.json
that is passed to Graal, which allows us to target classes in third party dependencies. If those classes were in a Quarkus app or directly in this project, we could use annotations such as @RegisterForReflection
on the class to do so.
Some other reflection APIs are automatically analyzed by Graal and do not need manual configuration (link to docs).
There is a Jackson extension for Quarkus that does extra processing and produces an ObjectMapper
synthetic bean like for our Cluster
, but we would have to explicitly use it in the SDK and configure it too. I haven't gone down that path as it seemed it wasn't needed for us.
would it be simple to preserve reflection info for all classes in a package?
That's interesting, I guess so? It would increase the size of the native-image, I'm not sure by how much.
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.
Thank you for the explanation @emilienbev . Tbh I continue to be rather horrified by the potential maintenance burden of this extension (I know you've put a lot of work into that already), and the optimise-at-any-maintenance-cost philosophy taken by Graal and its closed-system assumptions.
Can we schedule some time to look at solutions, particularly that suggestion of just including all reflection info? I would really like to get this extension safe and easily maintainable first (plus have it not be such a time sink for you), and look at what-I-suspect-will-be-micro optimisations later, if there is uptake that warrants it.
Happy to help if needed, though I think I will just get in your way.
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.
Yep would love to chat with you on this. I could use your input on both maintaining this extension, and maintaining the FIT performer which does not tolerate change well.
...t/src/main/java/com/couchbase/quarkus/extension/deployment/nettyhandling/NettyProcessor.java
Show resolved
Hide resolved
...t/src/main/java/com/couchbase/quarkus/extension/deployment/nettyhandling/NettyProcessor.java
Outdated
Show resolved
Hide resolved
...ests/src/main/java/com/couchbase/quarkus/extension/it/CouchbaseQuarkusExtensionResource.java
Show resolved
Hide resolved
runtime/src/main/java/com/couchbase/quarkus/extension/runtime/CouchbaseRecorder.java
Outdated
Show resolved
Hide resolved
runtime/src/main/java/com/couchbase/quarkus/extension/runtime/health/CouchbaseReadyCheck.java
Show resolved
Hide resolved
…e initialization.
...oyment/src/main/java/com/couchbase/quarkus/extension/deployment/CouchbaseDevUiProcessor.java
Outdated
Show resolved
Hide resolved
… the Cluster, added histogram configuration.
...oyment/src/main/java/com/couchbase/quarkus/extension/deployment/CouchbaseDevUiProcessor.java
Outdated
Show resolved
Hide resolved
|
||
@BuildStep | ||
ReflectiveClassBuildItem reflection() { | ||
return ReflectiveClassBuildItem.builder( |
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.
Thank you for the explanation @emilienbev . Tbh I continue to be rather horrified by the potential maintenance burden of this extension (I know you've put a lot of work into that already), and the optimise-at-any-maintenance-cost philosophy taken by Graal and its closed-system assumptions.
Can we schedule some time to look at solutions, particularly that suggestion of just including all reflection info? I would really like to get this extension safe and easily maintainable first (plus have it not be such a time sink for you), and look at what-I-suspect-will-be-micro optimisations later, if there is uptake that warrants it.
Happy to help if needed, though I think I will just get in your way.
runtime/src/main/java/com/couchbase/quarkus/extension/runtime/CouchbaseRecorder.java
Outdated
Show resolved
Hide resolved
…cs configuration. Removed try/catch in connection string helper for the Dev UI.
Motivation
Upgrade the extension to the latest Quarkus LTS and latest Java SDK release.
Changes
Bump Quarkus to 3.17.5
Bump Java SDK to 3.7.7
Update netty substitutions. As the official netty extension targets Netty 4.1.111.Final and our SDK uses 4.1.112.Final, some substitutions have been added to the "delete" step of the shell script.
(Specific change affecting substitutions between 4.1.111.Final and 4.1.112.Final)
Update: Upgrade Netty substitutions to 4.1.115.Final, SDK and Quarkus are aligned.
The legacy
@ConfigRoot
approach for CouchbaseConfig was changed to the new@ConfigMapping
one.Added the compiler arg
-AlegacyConfigRoot=true
to the deployment module pom.xml, as the netty substitution still uses it.Updated the way CouchbaseDevService gets the config fields.
Added an optional Readiness health check, with a configurable timeout
quarkus.couchbase.health.readiness.timeout
.Added support for emitting SDK metrics via the Couchbase
metrics-micrometer
package andquarkus-micrometer
.Native-image
The native image failed upon calling Cluster-related code due to
MpsArrayQueue
inDefaultEventBus
,OrphanReporter
andThresholdLoggingTracer
.MpscAtomicUnpaddedArrayQueue