ISPN-10322 Create unified interface for initializing commands #6834

ryanemerson · 2019-04-04T10:45:16Z

@danberindei @wburns This is an attempt to standardise the way in which we initialise our RPC commands to remove the need for the switch statement in CommandsFactoryImpl. This started when I was bored at the airport, so feel free to tear it apart 😄.

In order to avoid creating a sub-interface, e.g. ComandInitialisationContext, the InitializableCommand#init method takes in the ComponentRegistry and I have cached the components required throughout core to avoid hash lookups. The Expose* commits, was just a logical way of exposing sub-components by managers in order to reduce the amount directly expose by the ComponentRegistry. I'm not sure whether this is a good/bad idea.

I have left support for ModuleCommandInitializer as this can be more powerful e.g. hibernate CacheCommandInitializer. However, if we go with this approach we should probably all ModuleCommandExtensions#getModuleCommandInitializer to return null or provide a default impl that does this.

~~I've only update the core module for now, if this is something to pursue then I will update the other modules that utilise ModuleCommandInitializer.~~

https://issues.jboss.org/browse/ISPN-10322

galderz

LGTM. Just a minor comment about the function instance variable.

core/src/main/java/org/infinispan/commands/functional/AbstractWriteKeyCommand.java

ryanemerson · 2019-05-24T14:46:10Z

There's still a lot of failures on this branch, but I don't want to waste time resolving them until the general idea is approved. If not, I'll just close the PR.

tristantarrant · 2019-06-03T13:23:02Z

Overall I like this approach a lot.

wburns · 2019-06-05T13:00:31Z

Overall I like the approach as well as it keeps the code cleaner. I would love to see the perf difference; I am guessing it is negligible though (since it is a megamorphic call now instead but doesn't have the switch statement).

ryanemerson · 2019-06-18T15:36:49Z

Updated to apply the approach to all of our modules. @danberindei please take a look when you get a chance as I'm sure you'll be more interested then most.

ryanemerson · 2019-06-18T17:39:02Z

run performance tests please

ghost · 2019-06-18T18:22:21Z

Performance tests didn't finish successfully. @diegolovison, can you review it?

Additional info:
Commit: 23f63db
Build number: #345
Comment body: run performance tests please
skip ci

diegolovison · 2019-06-18T18:32:56Z

@ryanemerson blocked by radargun/radargun#603

diegolovison · 2019-06-19T13:07:52Z

run performance tests please

ghost · 2019-06-19T15:42:57Z

Performance tests run successfully. Link to the results here.

Additional info:
Commit: 23f63db
Build number: #346
Comment body: run performance tests please
skip ci

danberindei · 2019-06-21T08:37:14Z

@ryanemerson I would say it's a step in the right direction, but it doesn't go far enough!

I would like to get rid of the separate remote initialization step completely and instead add a method CacheRpcCommand.invoke(ComponentRegistry). We don't need the isRemote flag because we already have getOrigin() != null.

For local invocation, the invoker already knows the concrete type, so instead of adding a LocalInvocation layer it should call a concrete method that doesn't need the entire component registry and response unwrapping. Besides, LocalInvocation is blocking, so it should go away anyway. But I think this can be done in a separate PR.

Currently global commands use wireDependencies(), I'd like to get rid of that in the near future and create a GlobalRpcCommand interface with an invoke(GlobalComponentRegistry) method.

Further out, I hope to replace the interface invoke with an annotation, so commands can declare the components they need as parameters, and generated code takes care of both injection and caching, then I can finally retire ComponentRegistry :) It will require one hash map lookup to get the accessor, but I think it's worth it.

danberindei

Oops, forgot to press submit

danberindei · 2019-06-21T07:12:24Z

...e-v51/src/main/java/org/infinispan/hibernate/cache/v51/impl/BaseTransactionalDataRegion.java

@@ -156,17 +155,12 @@ private void prepareForValidation() {
      // Besides, any cache interceptor initialization should only be done once.
      // Synchronizes on the cache instance since it's shared between regions with same name.
      synchronized (cache) {
-         PutFromLoadValidator found = findValidator(cache);
+         PutFromLoadValidator found = cache.getComponentRegistry().getComponent(PutFromLoadValidator.class);
         validator = found != null ? found : new PutFromLoadValidator(cache, factory, factory.getPendingPutsCacheConfiguration());


This seems wrong, either the PutFromLoadValidator is supposed to be registered in the component registry and shared with others using the same cache, or it's not supposed to be registered and we should just create an instance here.

danberindei · 2019-06-21T07:19:00Z

query/src/main/java/org/infinispan/query/indexmanager/AbstractUpdateCommand.java

+   public void init(ComponentRegistry componentRegistry, boolean isRemote) {
+      Cache cache = componentRegistry.getCache().wired();
+      searchFactory = ComponentRegistryUtils.getSearchIntegrator(cache);
+      queryInterceptor = ComponentRegistryUtils.getQueryInterceptor(cache);


This is what I don't like about having an interface: ComponentRegistry can only cache core components, so other modules have to go back to hash map lookups. The effect on performance may be close to nil, but still it's annoying.

danberindei · 2019-06-21T07:32:21Z

core/src/main/java/org/infinispan/commands/tx/AbstractTransactionBoundaryCommand.java

+      this.invoker = componentRegistry.getInterceptorChain().running();
+      this.icf = componentRegistry.getInvocationContextFactory().running();
+      this.txTable = componentRegistry.getTransactionTableRef().running();
+      markTransactionAsRemote(isRemote);


This is the only place where we need the isRemote parameter. I'd rather remove the remote flag from GlobalTransaction, TBH it always bugged me that the identifier of a transaction is different depending on who you're asking.

danberindei · 2019-06-21T08:18:30Z

core/src/test/java/org/infinispan/commands/module/TestModuleCommandExtensions.java

-   @Override
-   public ModuleCommandInitializer getModuleCommandInitializer() {
-      return new ModuleCommandInitializer() {
-         @Inject EmbeddedCacheManager cacheManager;


As you're removing implementations from other modules I'd rather extend the test implementation here to add a custom command and verify that it keeps working.

server/hotrod/src/main/java/org/infinispan/server/hotrod/LifecycleCallbacks.java

…ializableCommand

…Command

danberindei · 2019-06-21T15:03:22Z

Integrated, thanks Ryan!

ryanemerson added the Preview label Apr 4, 2019

galderz approved these changes May 20, 2019

View reviewed changes

core/src/main/java/org/infinispan/commands/functional/AbstractWriteKeyCommand.java Outdated Show resolved Hide resolved

ryanemerson force-pushed the init_commands_2 branch from b5ae46e to e7eabfc Compare May 22, 2019 10:23

ryanemerson force-pushed the init_commands_2 branch 4 times, most recently from 16e9829 to e1cdfda Compare June 18, 2019 10:38

ryanemerson force-pushed the init_commands_2 branch 2 times, most recently from bad32e1 to bb9c7d8 Compare June 18, 2019 15:30

ryanemerson removed the Preview label Jun 18, 2019

ryanemerson force-pushed the init_commands_2 branch from bb9c7d8 to 23f63db Compare June 18, 2019 15:30

ryanemerson changed the title ~~Initializable Commands~~ ISPN-10322 Create unified interface for initializing commands Jun 18, 2019

ryanemerson added this to the 10.0.0.Beta4 milestone Jun 18, 2019

ryanemerson force-pushed the init_commands_2 branch from 23f63db to 6906799 Compare June 20, 2019 09:11

ryanemerson force-pushed the init_commands_2 branch from 6906799 to d467f17 Compare June 21, 2019 13:16

danberindei reviewed Jun 21, 2019

View reviewed changes

ryanemerson force-pushed the init_commands_2 branch from d467f17 to e32b3ec Compare June 21, 2019 14:37

ryanemerson added 3 commits June 21, 2019 15:43

ISPN-10322 Create unified interface for initializing commands

2b1f806

ISPN-10322 Remove hibernate CacheCommandInitializer in favour of Init…

d53ba2a

…ializableCommand

ISPN-10322 Remove query CommandInitializer in favour of Initializable…

fc15200

…Command

ISPN-10322 Remove HotRodCommandInitializer in favour of Initializable…

6c6ae36

…Command

ryanemerson force-pushed the init_commands_2 branch from e32b3ec to 6c6ae36 Compare June 21, 2019 14:43

danberindei merged commit eb3386a into infinispan:master Jun 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-10322 Create unified interface for initializing commands #6834

ISPN-10322 Create unified interface for initializing commands #6834

ryanemerson commented Apr 4, 2019 •

edited

galderz left a comment

ryanemerson commented May 24, 2019

tristantarrant commented Jun 3, 2019

wburns commented Jun 5, 2019

ryanemerson commented Jun 18, 2019

ryanemerson commented Jun 18, 2019

ghost commented Jun 18, 2019

diegolovison commented Jun 18, 2019

diegolovison commented Jun 19, 2019

ghost commented Jun 19, 2019

danberindei commented Jun 21, 2019

danberindei left a comment

danberindei Jun 21, 2019

danberindei Jun 21, 2019

danberindei Jun 21, 2019

danberindei Jun 21, 2019

danberindei commented Jun 21, 2019

ISPN-10322 Create unified interface for initializing commands #6834

ISPN-10322 Create unified interface for initializing commands #6834

Conversation

ryanemerson commented Apr 4, 2019 • edited

galderz left a comment

Choose a reason for hiding this comment

ryanemerson commented May 24, 2019

tristantarrant commented Jun 3, 2019

wburns commented Jun 5, 2019

ryanemerson commented Jun 18, 2019

ryanemerson commented Jun 18, 2019

ghost commented Jun 18, 2019

diegolovison commented Jun 18, 2019

diegolovison commented Jun 19, 2019

ghost commented Jun 19, 2019

danberindei commented Jun 21, 2019

danberindei left a comment

Choose a reason for hiding this comment

danberindei Jun 21, 2019

Choose a reason for hiding this comment

danberindei Jun 21, 2019

Choose a reason for hiding this comment

danberindei Jun 21, 2019

Choose a reason for hiding this comment

danberindei Jun 21, 2019

Choose a reason for hiding this comment

danberindei commented Jun 21, 2019

ryanemerson commented Apr 4, 2019 •

edited