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
Enable SPI functionality in GraalVM native mode #20
Conversation
116a9e1
to
ed60e22
Compare
addc4e5
to
2737b04
Compare
bf92d7b
to
66b4206
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.
Thanks for the PR. Added a few comments. Also, would it be possible to add unit tests for the classes you added?
README.md
Outdated
@@ -78,3 +78,4 @@ To make testing simple, the extension provides the `HazelcastServerTestResource` | |||
## Limitations (native mode) | |||
- Default Java serialization is not supported | |||
- User code deployment is not supported | |||
- Hazelcast SPI support is limited |
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.
Could you mention how it's limited? Does Kubernetes work? Do custom SPIs work?
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.
To be honest, it's hard to define. The code from parse
method is effectively a normal IMDG code stripped from some magic needed for the sake of OSGi and Glassfish compatibility so I expect some problems in these areas provided that you can make GraalVM work with that.
Naturally, all standard SPI-enabled extensions provided at build time do work properly
private ServiceLoaderUtils() { | ||
} | ||
|
||
static Set<Target_ServiceDefinition> parse(URL url, ClassLoader classLoader, ILogger logger) { |
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.
- Could you add javadoc explaining what this method does? Or maybe make this method name more explanatory?
- Wouldn't it be worth to add some unit tests for his class and other you added?
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 docs around the Graal substitution and explain the problem there as well.
I'm working on tests at the moment. Unit testing is useless, we need an integration test that loads a custom extension via SPI
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.
Actually, this is covered by Hasan's test suite
9531e25
to
a2cc065
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.
Finally we have got green build 💚 🙂 👍
http://jenkins.hazelcast.com/view/Integration%20Tests/job/kubernetes-quarkus-client-extension-aggregate-spis-branch/6/
15:21:13 Entry added into map succesfully!
15:21:14 Extracted entry value is correct.
15:21:14 {"podName":"hazelcast-client-0","value":"value_1"}
15:21:16 {"podName":"hazelcast-client-2","value":"value_1"}
15:21:19 {"podName":"hazelcast-client-2","value":"value_1"}
15:21:21 {"podName":"hazelcast-client-2","value":"value_1"}
15:21:23 {"podName":"hazelcast-client-0","value":"value_1"}
15:21:25 Stopping Docker container after build completion
15:21:26 Finished: SUCCESS
…queness for their content instead (#16890) We encountered a problem related to the fact that resource URLs(obtained via `ClassLoader.getResources`) can sometimes be not-unique(under GraalVM native images, for example) but lead to unique content anyway. It might increase resilience long-term, but `quarkus-hazelcast-client` doesn't really rely on this change. Details: hazelcast/quarkus-hazelcast-client#20 Co-authored-by: Tomasz Gawęda <tomasz.gaweda@outlook.com>
This change introduces a GraalVM substitution for
ServiceLoader#getServiceDefinitions
so that discovered classpath resources are properly discovered, read and not accidentally discarded.Motivation
Hazelcast SPI relies on the classpath presence of multiple META-INF/services/com.foo.Bar files (for example, com.hazelcast.spi.discovery.multicast.DiscoveryStrategyFactory).
The internal implementation of ServiceLoader processes and collects URLs to the classpath resources:
https://github.com/hazelcast/hazelcast/blob/d54fa6df9a88c59f7479744c2b46158f10b2df7f/hazelcast/src/main/java/com/hazelcast/internal/util/ServiceLoader.java#L101-L124
However, this implementation doesn't play along with GraalVM's native images since Substrate doesn't really feature a normal classpath nor classloading but a mere simulation of these.
URLs to classpath resources located in different jars normally look like:
but under Substrate, they become resources and share the same URL (but
URL#openStream
leads to different content) which requires special handling:So, not only extra processing and conversions are not welcome:
https://github.com/hazelcast/hazelcast/blob/d54fa6df9a88c59f7479744c2b46158f10b2df7f/hazelcast/src/main/java/com/hazelcast/internal/util/ServiceLoader.java#L109-L112
But also storing URLs in a set causes rejection of valid URLs:
https://github.com/hazelcast/hazelcast/blob/d54fa6df9a88c59f7479744c2b46158f10b2df7f/hazelcast/src/main/java/com/hazelcast/internal/util/ServiceLoader.java#L106
fixes #18