Infinispan Embedded Extension #3832

wburns · 2019-09-03T17:26:09Z

Adds in initial support to get Infinispan Embedded working as an extension in Quarkus when running in native mode.

This PR only includes Infinispan core and thus querying is not yet included.

FroMage · 2019-09-04T15:02:36Z

@Sanne would you be able to review this?

machi1990 · 2019-09-06T15:20:39Z

Fixes #3617

Sanne · 2019-10-07T09:22:14Z

hey @wburns - sorry it took so long, it's failing now if I try a rebase. I guess something changed in the meantime:

[ERROR] Failed to execute goal io.quarkus:quarkus-maven-plugin:0.10.0:build (default) on project quarkus-integration-test-infinispan-embedded: Failed to build a runnable JAR: Failed to build a runner jar: Failed to augment application classes: Build failure: Build failed due to errors
[ERROR] 	[error]: Build step io.quarkus.deployment.steps.ConfigurationSetup#initializeConfiguration threw an exception: java.lang.IllegalArgumentException: No Converter registered for class class io.quarkus.runtime.configuration.MemorySize

Sanne

Looks pretty good, although it doesn't take advantage of many more optimisations we could do. I wonder if you have plans for later, or if I'm missing something about the use case.

Some changes are necessary though at least to make it work - see my PR comment #3832 (comment)

(and since it didn't build for me I couldn't actually try it out in native yet)

Sanne · 2019-10-07T09:39:39Z

extensions/infinispan-embedded/deployment/pom.xml

@@ -0,0 +1,66 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ Copyright 2019 Red Hat, Inc.


You might want to skip the copyright headers.

Oh nice, didn't know we can get rid of them now :)

Sanne · 2019-10-07T09:42:24Z

...ent/src/main/java/io/quarkus/infinispan/embedded/deployment/InfinispanEmbeddedProcessor.java

+import io.quarkus.infinispan.embedded.runtime.InfinispanRecorder;
+
+class InfinispanEmbeddedProcessor {
+    @BuildStep


Just wondering, why do we need to index all these components?

Are we unable to "pre-start" the internal registries and metadata - wouldn't that make this unnecessary?

The extension will need to be able to read in a configuration file at runtime, thus we need to be able to have all the required classes available via reflection. JGroups has every protocol (50+) and property required for reflection. Infinispan needs to have quite a few classes as well. The reason then for using the index, is so I don't have to hard code every single one of these classes in the extension, which would be prone to breakage, especially as we don't control JGroups.

Sanne · 2019-10-07T09:44:14Z

...ent/src/main/java/io/quarkus/infinispan/embedded/deployment/InfinispanEmbeddedProcessor.java

+    void allowReflectionForTransactions(Capabilities capabilities, BuildProducer<ReflectiveClassBuildItem> reflectiveClass) {
+        if (capabilities.isCapabilityPresent(Capabilities.TRANSACTIONS)) {
+            // Our JBossStandaloneJTAManagerLookup uses reflection on these
+            reflectiveClass.produce(new ReflectiveClassBuildItem(true, false, "com.arjuna.ats.jta.TransactionManager"));


you could make these unnecessary, just include a TXM lookup implementation which depends on the module?

Not sure I understand sorry. I didn't see any module that registers this via reflection. Although the more I look at it I can just use a substrate substitution instead of this.

Sanne · 2019-10-07T09:46:27Z

...ent/src/main/java/io/quarkus/infinispan/embedded/deployment/InfinispanEmbeddedProcessor.java

+            ServiceLoader<?> serviceLoader = ServiceLoader.load(serviceLoadedInterface);
+            List<String> interfaceImplementations = new ArrayList<>();
+            serviceLoader.forEach(mmb -> interfaceImplementations.add(mmb.getClass().getName()));
+            if (!interfaceImplementations.isEmpty()) {


Ok this makes me a bit sad :-/ Can't you prepare the bootstrap of the EmbeddedCacheManager upfront?

Such operations like looking up components shouldn't belong in the "runtime phase".

Of course my comment isn't a blocker - but I wonder if you know about this and if there's plans to improve on this in a second pass?

I am not sure how we could at this point. The ModuleMetadataBuilder is only loaded up via service loader currently and it pretty ingrained in. Unless I guess I could remove this code and add in some substrate substitutions to change constructors to reference statically defined variables instead. I am not 100% sure on that though, and I think we can add that in later if needed.

Sanne · 2019-10-07T09:48:46Z

...ent/src/main/java/io/quarkus/infinispan/embedded/deployment/InfinispanEmbeddedProcessor.java

+        addReflectionForClass(PropertyConverter.class, true, combinedIndex, reflectiveClass, excludedClasses);
+
+        // Just add the JBossAS and Embedded TransactionManagerLookups
+        reflectiveClass.produce(new ReflectiveClassBuildItem(false, false, JBossStandaloneJTAManagerLookup.class));


I think you should choose one strategy and stick to it. Do we really need both JBossStandaloneJTAManagerLookup and EmbeddedTransactionManagerLookup ?

Also, I think you should auto-inject the right choice - right now I see the documentation require the user to set one, this should be unnecessary if you can require the single choice of the platform.

The JBossStandaloneJTAManagerLookup is so the user can link Infinispan with the transaction manager in Quarkus. The EmbeddedTransactionManagerLookup is if they just want a simple transaction manager to use outside of it.

Also, I think you should auto-inject the right choice - right now I see the documentation require the user to set one, this should be unnecessary if you can require the single choice of the platform.

Do you mean to say we should only allow the Quarkus transaction manager? Then the user wouldn't have to configure that. This would require the transaction extension always then. The embedded one is quite light weight in comparison.

Yes support only "the" transaction manager provided by the platform.

Many optimisations are made possible only because we give up on allowing people to use any weird and crazy replacement for core components: it also helps quality & people as we're accountable for the choice of implementations being made.

👍 that makes my life easier as well

Sanne · 2019-10-07T11:01:43Z

...bedded/runtime/src/main/java/io/quarkus/infinispan/embedded/runtime/graal/FixJMXClasses.java

+
+    @Substitute
+    protected synchronized void addShutdownHook() {
+        // Don't do anything or do we want to?


probably best to log a warning.

Looking closer, just doing nothing should be acceptable.

Sanne · 2019-10-07T11:02:50Z

...untime/src/main/java/io/quarkus/infinispan/embedded/runtime/graal/JChannelSubstitutions.java

+
+/**
+ * These substitutions need to be revisited on each new Quarkus/GraalVM release, as some methods may become
+ * supported. The better option currently is to use -H:+ReportUnsupportedElementsAtRuntime, which moves potential issue


I wouldn't suggest that as as "better option" YMMV

This is just copied from Bela's stuff :) I can remove the comment.

yes please remove :)

Sanne · 2019-10-07T11:04:23Z

integration-tests/infinispan-embedded/pom.xml

+        <!-- Test Dependencies - Note this is not test scope as native compilation doesn't use test classes/dependencies -->
+        <dependency>
+            <groupId>io.quarkus</groupId>
+            <artifactId>quarkus-narayana-jta</artifactId>


should the quarkus-narayana-jta not be a mandated dependency? Can it run without it?

Sure, if you don't have any transactional caches it works just fine. But now that I know I should force users to only use Quarkus transaction manager I can add some more logic around this and some checks.

Please verify: does native compilation still work fine whtn the TXM is missing? I bet it won't compile ;)

It passes fine when I comment out this line and the testTransactionRolledBack test that failed without it.

INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.426 s - in io.quarkus.it.infinispan.embedded.InfinispanClientFunctionalityInGraalITCase

Sanne · 2019-10-07T11:06:18Z

integration-tests/infinispan-embedded/pom.xml

+                                    <dumpProxies>false</dumpProxies>
+                                    <!-- We have some example stack files we want to use -->
+                                    <additionalBuildArgs>
+                                        <additionalBuildArg>-H:ResourceConfigurationFiles=${project.basedir}/src/main/resources/resources-config.json</additionalBuildArg>


shouldn't the configured stack be picked up automatically for inclusion by your extension?

I think this could hide some issues.

We can't because these stacks are referenced at runtime via the xml file.

Sanne · 2019-10-07T11:07:08Z

integration-tests/infinispan-embedded/src/main/resources/resources-config.json

@@ -0,0 +1,7 @@
+{


See related comment in the native-image section of the integration test's pom. I think this file should be removed.

wburns · 2019-10-07T14:07:12Z

hey @wburns - sorry it took so long, it's failing now if I try a rebase. I guess something changed in the meantime:

[ERROR] Failed to execute goal io.quarkus:quarkus-maven-plugin:0.10.0:build (default) on project quarkus-integration-test-infinispan-embedded: Failed to build a runnable JAR: Failed to build a runner jar: Failed to augment application classes: Build failure: Build failed due to errors
[ERROR] 	[error]: Build step io.quarkus.deployment.steps.ConfigurationSetup#initializeConfiguration threw an exception: java.lang.IllegalArgumentException: No Converter registered for class class io.quarkus.runtime.configuration.MemorySize

Weird it compiles just fine for me. But I didn't rebase on master yet, so it is probably related to that :) I will do that next.

wburns · 2019-10-07T14:10:29Z

Looks pretty good, although it doesn't take advantage of many more optimisations we could do. I wonder if you have plans for later, or if I'm missing something about the use case.

Yeah, this is the first pass only at this point. I wanted to get something that worked with configuration file that had all the options. We can trim back things and add in additional optimizations later, at least that was my thought.

Sanne · 2019-10-07T15:08:16Z

thanks @wburns . Will you rebase and test that please?

wburns · 2019-10-07T15:35:24Z

@Sanne Yeah, it just finished.

Everything included native passed for me:

[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.599 s - in io.quarkus.it.infinispan.embedded.InfinispanClientFunctionalityInGraalITCase

wburns · 2019-10-07T15:56:17Z

Only other thing I can think of is I am using 19.2.0. Let me try rerunning tests with 19.2.0.1 and see if it causes them to fail.

wburns · 2019-10-07T20:12:58Z

@Sanne fixed the comments I could, rebased, and tested and everything passed for me.

Sanne · 2019-10-08T08:42:18Z

Thanks @wburns . Trying again, let's see if I can figure it out quickly - I hope so as I'm supposed to work on some other urgent stuff :/

Sanne · 2019-10-08T09:25:03Z

@wburns could you tell me which version of the plugin you have logged when invoking

 mvn io.quarkus:quarkus-maven-plugin:help

?

In my case it defaults to a very old version. So I think I have two problems:

why is my system default using a very old tag (and this is probably unrelated from your PR)
why is it not using the project version when I'm testing your module (could be related to your PR?)

Sanne · 2019-10-08T10:02:09Z

@wburns I pushed two additional commits, the second one ensures the right version of the plugin is picked up.

Sanne · 2019-10-08T10:04:39Z

+                     <stack name="tcpping" extends="tcp">
+                         <MPING ispn:stack.combine="REMOVE" xmlns="urn:org:jgroups"/>
+                         <TCPPING async_discovery="true"
+                                  initial_hosts="${initial_hosts:192.168.1.20[7800],192.168.1.20[7801]}"

Infinispan Embedded Extension #3832

Infinispan Embedded Extension #3832

Conversation

wburns commented Sep 3, 2019

FroMage commented Sep 4, 2019

machi1990 commented Sep 6, 2019

Sanne commented Oct 7, 2019

Sanne left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wburns commented Oct 7, 2019

wburns commented Oct 7, 2019

Sanne commented Oct 7, 2019

wburns commented Oct 7, 2019

wburns commented Oct 7, 2019

wburns commented Oct 7, 2019

Sanne commented Oct 8, 2019

Sanne commented Oct 8, 2019

Sanne commented Oct 8, 2019 • edited Loading

Sanne commented Oct 8, 2019

wburns commented Oct 8, 2019

Sanne commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wburns commented Oct 10, 2019

Sanne left a comment •

edited

Loading

Sanne commented Oct 8, 2019 •

edited

Loading