-
Notifications
You must be signed in to change notification settings - Fork 564
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
4.x Eclipse Store integration #8269
base: main
Are you sure you want to change the base?
Conversation
@fh-ms @zdenek-jonas - this is the DRFAT merge request. ^^^^ Question for the Helidon team: |
EclipseStore 1.1.0 is available. @hg-ms and I did a code review. Looks good for us. |
Thank you @fh-ms and @hg-ms for reviewing the code! The only additional commit I did was to change the versions to 1.1.0 @tjquinno @ljnelson @barchetta Any clue in which Helidon version it could be included, assuming we missed the 4.0.4 train? |
@fh-ms
|
@fh-ms @zdenek-jonas. Please review the latest Eclipse Store 1.2.0 upgrade to this pull request. A couple of things:
|
c50d584
to
c92e8fd
Compare
I did a review of the EclipseStore specific parts, everything is ok from our side. Regarding Aot support: it should work but requires additional user actions because all user defined classes that are going to be persisted must be accessible by reflections. I’d suggest to add a description to @aot documenting this. As we don’t have a replacement for the javax.cache API we need to live with that. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
3a35cd5
to
a8c2aa9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
a8c2aa9
to
8beaf7f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
8beaf7f
to
4c18977
Compare
4c18977
to
1eb9619
Compare
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.
Can you add an example ?
import static org.hamcrest.MatcherAssert.assertThat; | ||
|
||
// TODO disabled | ||
@Disabled |
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.
Why is this test disabled ?
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.
The equivalent Microstream test was disabled om 9/5/2023 by Tim Quinn. I will test if it can be re-enabled .
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 will add examples, but a bit later tonight
@romain-grecourt plese review the new commits/ run the pipeline. |
...c/main/java/io/helidon/examples/integrations/eclipsestore/greetings/mp/GreetingProvider.java
Outdated
Show resolved
Hide resolved
@romain-grecourt assuming we square away all little things today, what is a realistic date where this merge can be relased as part of an official Helidon - 4.0.7? 4.1.0? This pull request is 3 months old already. |
73fb1c8
to
6fce3df
Compare
Thanks for this contribution. We are now making some decisions regarding the timing. I want to spend some time on this to use our new Builder APIs and do a bit of Helidon alignment, will let you know asap |
@tomas-langer thanks! Depending on my time availability (mostly my free time), I would also be making some final code review for this contribution (particularly whether I implemented correctly all ES 1.2.0 config options), so no rush ... |
@fh-ms @zdenek-jonas - I noticed that EclipseStore 1.3.1 came out yesterday. And siince this pull request is still not approved yet, maybe it is better to upgrade it to EclipseStore 1.3.1 anyway? I also noticed that EclipseStore 1.3.1 provides CDI4 integration now "out-of-the-box". This potentially means that Helidon MicroProfile (MP) will no longer need the CDI and Cache modules, and they can be removed - do you agree? My only gripe with the new ES 1.3.1 CDI and Cache code is that it is missing module-info.java - any chance that can be fixed quickly? @romain-grecourt @tomas-langer - please refrain from merging this pull request just yet, until we hear from the Microstream team - at the very least we need to upgrade from 1.2.0 to 1.3.1 |
An update to 1.3.1 makes sense due to an adjustment for Java 22. We still have to test the CDI stuff with Helidon properly, and maybe we find a way to add the module-infos there, so we should wait until we remove the modules. |
Regarding CDI integration - I think the Helidon MP integration is not "just" CDI, but it should also support MP Config as a source of configuration data. If that is the case, it may still be useful. If not, then I agree it can be removed and we can use the built-in CDI integration. |
Description
Add Eclipse Store support to Helidon.
Eclipse Store is the successor of Microstream, both of which will be available in Helidon 4.x (once this request is merged) enabling smooth transition.
This pull request targets Eclipse Store 1.2.0 and upgrades Microstream to 08.01.02-MS-GA
Documentation
The official Eclipse Store documentation can be found here.
If no doc impact: None