ISPN-11541 Add internal metadata to write commands #8113

pruivo · 2020-03-26T18:27:16Z

https://issues.redhat.com/browse/ISPN-11541

tristantarrant · 2020-04-07T07:44:09Z

Should we perfack this ?

ryanemerson · 2020-04-07T09:53:29Z

@pruivo Is this PR still valid in light of #7858 (comment)?

Probably, it is best just to create a simple class with 2 fields (irac version and opt-tx version)

pruivo · 2020-04-07T10:50:18Z

@pruivo Is this PR still valid in light of #7858 (comment)?
Probably, it is best just to create a simple class with 2 fields (irac version and opt-tx version)

yes. I have already the InternalCacheEntry updated and if I change the class, it would break this PR and create a bunch of conflicts in the next ones.
I would rather have everything integrated with MetaParamsInternalMetadata and change it later in one go.

pruivo · 2020-04-07T10:51:25Z

run performance tests please

ryanemerson · 2020-04-07T11:30:49Z

I would rather have everything integrated with MetaParamsInternalMetadata and change it later in one go.

How far down the line are we talking? This touches a lot of classes, so it makes it somewhat of a moving target for us working in the same area.

pruivo · 2020-04-07T11:34:38Z

The persistence is missing. My plans are to open a PR after this one.
It includes updating MarshallableEntry and the CacheStore implementations

ryanemerson · 2020-04-07T13:11:31Z

core/src/main/java/org/infinispan/factories/InterceptorChainFactory.java

+      //TODO! why adding version for replace?
+      //if (!configuration.transaction().transactionMode().isTransactional()) {
+      //   interceptorChain.appendInterceptor(createInterceptor(new VersionInterceptor(), VersionInterceptor.class), false);
+      //}


Is this still meant to be commented?

If we're removing this, should it be a separate commit/Jira?

I forgot to ask @gustavonalle about it. Is the VersionInterceptor still needed?
It sets some version for the replace commands and I didn't find any usage of that version.
It breaks the xsite tests if non-tx cache (on site 1) backups to a tx cache (on site 2).

I am not sure TBH. This is a relic from the compat mode times

oh... I found a failure... EmbeddedRestMemcachedHotRodTest fails without it.
I need to drop the version in the receiver site... bah...

ryanemerson · 2020-04-07T13:24:50Z

core/src/main/java/org/infinispan/statetransfer/StateConsumerImpl.java

@@ -733,6 +733,7 @@ protected IntSet getOwnedSegments(ConsistentHash consistentHash) {
      InternalMetadataImpl metadata = new InternalMetadataImpl(e);
      PutKeyValueCommand put = commandsFactory.buildPutKeyValueCommand(e.getKey(), e.getValue(), segmentId,
                                                                       metadata, STATE_TRANSFER_FLAGS);
+      put.setInternalMetadata(e.getInternalMetadata());


We should add MetaParamsInternalMetadata to CommandsFactory#buildPutKeyValueCommand.

Similarly, I noticed that the internal metadata is not set in ProtobufMetadataManagerInterceptor. How are internal commands handled by xsite, should this be set there as well?

We should add MetaParamsInternalMetadata to CommandsFactory#buildPutKeyValueCommand.

I didn't add it because I want to try to avoid touching too much interface... it would require a lot of changes everywhere and most of the places, the value is null.

Similarly, I noticed that the internal metadata is not set in ProtobufMetadataManagerInterceptor.
Is it needed? and set it to what exactly? (sorry, no idea what ProtobufMetadataManagerInterceptor is)

The current xsite impl doesn't use it.
In the new one, the internal metadata is set by the primary owner before replicating it to the backup owners.
It is also used by the state transfer so the joiners can set the correct versions.

core/src/main/java/org/infinispan/xsite/ClusteredCacheBackupReceiver.java

wburns · 2020-04-07T13:00:44Z

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

@@ -20,6 +24,7 @@
   long flags;
   DataConversion keyDataConversion;
   DataConversion valueDataConversion;
+   Map<Object, MetaParamsInternalMetadata> internalMetadataMap;


Is there no way we can trim this somehow... a ConcurrentHashMap for every functional command is a lot of bloat. Why not just 1 like we did with the other metadata?

The internal metadata is stored per key. Each key will have a different version.

ryanemerson · 2020-04-07T13:37:29Z

The persistence is missing. My plans are to open a PR after this one.
It includes updating MarshallableEntry and the CacheStore implementations

I have looked through the code and for the most part it seems fine. However it is not easy reviewing knowing that nearly all of the changes in this PR are going to change. Let's get the the follow ups in quickly (I'll make sure I'm timely with a review). I'll hold off rebasing my marshalling branch until then and progress locally, as it's too much of a moving target otherwise due to all of the areas this touches. Plus a simple class containing the two versions will make my life simpler in the long run.

johnou · 2020-04-07T11:52:02Z

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

@@ -30,13 +35,15 @@ protected AbstractWriteManyCommand(CommandInvocationId commandInvocationId,
      this.flags = params.toFlagsBitSet();
      this.keyDataConversion = keyDataConversion;
      this.valueDataConversion = valueDataConversion;
+      this.internalMetadataMap = new ConcurrentHashMap<>();


Can we use an immutable empty list here?

no, otherwise I will get an UnsupportedException when I try to put metadata there.
It can be null but then I will have to put some null checks before get/put and do a proper synchronization around it. I would rather not do it.

pruivo · 2020-04-07T15:47:11Z

@ryanemerson updated!
thanks for the review.

ryanemerson · 2020-04-07T17:12:34Z

@diegolovison Perk ack didn't run for some reason. Can you take a look?

ghost · 2020-04-08T11:06:27Z

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

Additional info:
Commit: 38d1bd6
Build number: #428
Comment body: run performance tests please
skip ci

diegolovison · 2020-04-08T11:30:44Z

run performance tests please

ghost · 2020-04-08T11:34:00Z

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

Additional info:
Commit: c577dd6
Build number: #431
Comment body: run performance tests please
skip ci

diegolovison · 2020-04-08T11:50:53Z

run performance tests please

ghost · 2020-04-08T16:53:40Z

Performance tests run successfully. Link to the results here.

Additional info:
Commit: c577dd6
Build number: #433
Comment body: run performance tests please
skip ci

pruivo · 2020-04-08T17:25:02Z

perf looks good @ryanemerson

ryanemerson

Two very minor things. I'm looking at the follow up now

core/src/main/java/org/infinispan/remoting/responses/PrepareResponse.java

ryanemerson · 2020-04-14T10:32:21Z

Thanks @pruivo

pruivo force-pushed the t_commands_internal_metadata branch 2 times, most recently from ae96055 to 919a3c7 Compare March 30, 2020 08:55

pruivo force-pushed the t_commands_internal_metadata branch from 919a3c7 to 38d1bd6 Compare April 6, 2020 15:15

tristantarrant requested a review from ryanemerson April 7, 2020 07:44

ryanemerson reviewed Apr 7, 2020

View reviewed changes

wburns reviewed Apr 7, 2020

View reviewed changes

johnou suggested changes Apr 7, 2020

View reviewed changes

pruivo force-pushed the t_commands_internal_metadata branch from 38d1bd6 to 11252ee Compare April 7, 2020 15:46

pruivo force-pushed the t_commands_internal_metadata branch 2 times, most recently from d853df1 to c577dd6 Compare April 8, 2020 08:37

pruivo mentioned this pull request Apr 8, 2020

ISPN-11611 Add internal metadata to persistence #8180

Merged

pruivo mentioned this pull request Apr 8, 2020

ISPN-11615 Replace internal metadata class #8182

Merged

ryanemerson reviewed Apr 9, 2020

View reviewed changes

core/src/main/java/org/infinispan/remoting/responses/PrepareResponse.java Outdated Show resolved Hide resolved

core/src/main/java/org/infinispan/remoting/responses/PrepareResponse.java Outdated Show resolved Hide resolved

pruivo force-pushed the t_commands_internal_metadata branch from c577dd6 to 7438ff6 Compare April 9, 2020 09:24

ISPN-11541 Add internal metadata to write commands

2e3d496

pruivo force-pushed the t_commands_internal_metadata branch from 7438ff6 to 2e3d496 Compare April 13, 2020 10:19

ryanemerson merged commit f10d8b5 into infinispan:master Apr 14, 2020

pruivo deleted the t_commands_internal_metadata branch April 14, 2020 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-11541 Add internal metadata to write commands #8113

ISPN-11541 Add internal metadata to write commands #8113

pruivo commented Mar 26, 2020

tristantarrant commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

pruivo commented Apr 7, 2020

pruivo commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

pruivo commented Apr 7, 2020

ryanemerson Apr 7, 2020

pruivo Apr 7, 2020

gustavocoding Apr 7, 2020

pruivo Apr 7, 2020

ryanemerson Apr 7, 2020

pruivo Apr 7, 2020

wburns Apr 7, 2020

pruivo Apr 7, 2020

ryanemerson commented Apr 7, 2020

johnou Apr 7, 2020

pruivo Apr 7, 2020

pruivo commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

ghost commented Apr 8, 2020

diegolovison commented Apr 8, 2020

ghost commented Apr 8, 2020

diegolovison commented Apr 8, 2020

ghost commented Apr 8, 2020

pruivo commented Apr 8, 2020

ryanemerson left a comment

ryanemerson commented Apr 14, 2020

ISPN-11541 Add internal metadata to write commands #8113

ISPN-11541 Add internal metadata to write commands #8113

Conversation

pruivo commented Mar 26, 2020

tristantarrant commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

pruivo commented Apr 7, 2020

pruivo commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

pruivo commented Apr 7, 2020

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

ryanemerson commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pruivo commented Apr 7, 2020

ryanemerson commented Apr 7, 2020

ghost commented Apr 8, 2020

diegolovison commented Apr 8, 2020

ghost commented Apr 8, 2020

diegolovison commented Apr 8, 2020

ghost commented Apr 8, 2020

pruivo commented Apr 8, 2020

ryanemerson left a comment

Choose a reason for hiding this comment

ryanemerson commented Apr 14, 2020