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.
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
Ugrade to Quarkus 3.17.5 and Java SDK 3.7.7 #123
Changes from all commits
500fbdf
b33ec8f
8baa417
e81b03c
752d968
3c8a654
098d580
eeeebfa
427f7eb
a8ced2d
d8a8ec8
d113262
22db759
4ecb7b3
1b513ce
3f64ed9
936cd01
6cfaed9
fce8724
93f9a6e
6a16179
d40a7d6
1aac1cb
5e12446
a89791f
45ce195
b570585
5f076d8
7b6b3f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 inClassicCoreBucketManager.getAllBuckets()
for example. That gives the bucket from the http response as aJsonNode
to theObjectMapper
: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 markMapper.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.
Yes indeed. This list was built by running FIT, and seeing when an reflection error would happen and which operation it affected.
For Jackson data-binding this
ReflectiveClassBuildItem()
from Quarkus generates areflection-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 ourCluster
, 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.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.