feat(v2): GraalVM support for parameters module #1824
Conversation
|
I tried to follow the guide as mentioned by https://github.com/aws-powertools/powertools-lambda-java/blob/v2/GraalVM.md which helped a lot with generating the meta data. However I do wonder if we need all the metadata generated by the GraalVM agent. In on of my projects where I used v1 of Powertools I only needed the following snippet to get the SecretsProvider to work: Now we have all sorts of java internal classes or classes coming from other libraries or the JVM, which I don't think we need? Shouldn't they be handled by the corresponding projects? I'm by no means a GraalVM expert, but I always understood it in such a way that metadata in a provided library only contains metadata about its own classes and resources which otherwise can't be reached. |
|
Hi @jreijn, I just review your changes and you did an amazing job 🚀 – especially cleaning up the test scoped dependencies in the generated metadata files. This helped us a lot. I just verified that unit tests against the native image pass, which they do ✅ Before we go ahead merging this PR, I would love to do two things:
I am happy to also update the example and don't want to cause more work on your end. But let me know if you like to do it. Regarding your comment:
You are absolutely right here. Since we run the metadata generation via unit tests (which is uncommon) there is a risk of including "too much" metadata and this is something we can optimize in the future. This is also why the manual cleanup is needed. The unit tests are good candidate for now since they have pretty good coverage. One way to address this which I tested was including a filter on the GraalVM tracing agent to only generate metadata for the library itself and not dependencies. However, this led to two issues:
Having said that, there is a lot of potential on improving this and before going to production with It seems like in the parameters example that you also tested in |
|
@phipag thanks for the feedback! With regards to the two open items:
With regard to the reflection metadata I see your point. I'm fine with leaving it for now and improving it later. Once we get to that point I would love to brainstorm a bit about it and see how I can contribute. |
|
Thanks @jreijn, this sounds like a good plan. Awesome that you would like to update the example as well! I'll be waiting for your update here and try to review it promptly. Regarding the roadmap update, I will take care of this as part of a separate PR since I am already working on a different documentation update. Next steps: Wait for |
|
Hey @jreijn, |
|
@phipag I'm sorry for the delay. I will pick it up tomorrow! Sorry I had a busy couple of weeks! |
|
@jreijn, no problem, we all know these busy weeks. Thanks for your quick reply. I'll be waiting for your updates. |
|
@phipag I'm close to pushing my changes. Found some missing reflection config for the transformer that were not picked up by the tests. I'll try to push them tomorrow morning with working examples. |
|
@phipag I've added the working example and tested it (it took me a bit longer then expected). I did find something in parameter module that wasn't obvious. If you have some more time before the release I was thinking of moving the |
|
Thanks for your update @jreijn ! I will review it today. Regarding your suggestion, this is an interesting point. Especially the fact that the tests have a dedicated package but not the core logic. I don't see a reason (right now) why not do it. I will test this and might update it before the final v2 release (target for end of Q2).
What do you mean by "easy generation". Is it easier to generate the GRM files with that new structure? Edit: Ignore the failing workflows. It seems that GitHub is experiencing problems. https://www.githubstatus.com/ |
@phipag Right now I've added the graalvm maven profiles, to generate the metadata, to the powertools-parameters-test maven module and moved the generated metadata to the Merging tests and the src folder would allow for the same proces as in any other submodule of the project. |
...ecrets/src/main/java/software/amazon/lambda/powertools/parameters/secrets/SecretsParams.java
Outdated
Show resolved
Hide resolved
|
Hey @jreijn, I sent a few very small commits addressing some typos and other cosmetic things. I tested your example and deployed it in my AWS account and it works perfectly. I tested it both from Mac using podman compilation and also natively from an Amazon Linux machine. See my comment above regarding the |
…allow GitHub workflows to check minimum compatible Java version.
…rt' into feat/v2-parameters-graalvm-support
| @@ -1,5 +1,5 @@ | |||
| build-ParametersFunction: | |||
|
|
|||
There was a problem hiding this comment.
Nice catch! It seems I forgot to add it back after my final tests.
|
|
@phipag that's awesome! Can't wait to try it out! Will start working on the next GraalVM module! |
|
Looking forward to this! 🚀 |
Issue #, if available: #1833
Description of changes:
Introduced GraalVM metadata for all submodules of the parameters module:
Checklist
Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.