ISPN-7847 Deprecate DeltaAware #5187

rvansa · 2017-06-08T15:14:06Z

DeltaAware, CopyableDeltaAware, Delta, DeltaCompositeKey deprecated
AdvancedCache.applyDelta deprecated, CacheImpl.applyDelta reimplemented using ReadWriteKeyValueCommand, permits to lock only on the main key
ApplyDeltaCommand and Visitor.visitApplyDeltaCommand deprecated (implementation removed)
DeltaAwareCacheEntry, DeltaAwareObjectOutput and some internal helpers removed

tristantarrant · 2017-06-09T07:19:51Z

3 related failures

rvansa · 2017-06-09T20:15:11Z

I've removed the three failing tests; these are no longer applicable because Flag.DELTA_AWARE is no longer used, and does not make the put requiring previous value on backup owner.
Also adding back the Wait CI Results label - we need to ran whole testsuite (not just core) to check potential problems in tree cache or lucene directory.

galderz · 2017-06-15T09:27:28Z

Last build din't work, but there was a build from 2 days back that run fine. Do you want to wait for another CI run?

rvansa · 2017-06-15T15:31:58Z

@galderz Not really, but I see there is a conflict now :-/

rvansa · 2017-06-16T08:10:03Z

@galderz Rebased.

pruivo

just one small bug found :)
it looks good to me. It is good to see some code removed :) Nice job

pruivo · 2017-06-16T15:19:05Z

core/src/main/java/org/infinispan/cache/impl/CacheImpl.java

+      DataWriteCommand command;
+      if (value instanceof Delta) {
+         command = createApplyDelta(key, (Delta) value, explicitFlags, ctx);
+      } if (value instanceof DeltaAware && (explicitFlags & FlagBitSets.CACHE_MODE_LOCAL) == 0) {


bug: else is missing

well spotted! :)

pruivo · 2017-06-16T15:22:20Z

core/src/main/java/org/infinispan/atomic/impl/ApplyDelta.java

+            deltaAware = ((CopyableDeltaAware) value).copy();
+         } else if (value instanceof DeltaAware) {
+            try {
+               byte[] bytes = marshaller.objectToByteBuffer(value);


@rvansa Could it use MergeOnStore instead? Not that I think MergeOnStore is a good idea, but it seems quite close to the semantics of non-copyable DeltaAware.

pruivo · 2017-06-16T15:26:08Z

core/src/main/java/org/infinispan/interceptors/distribution/TxDistributionInterceptor.java

@@ -272,7 +267,7 @@ private Object handleSecondPhaseCommand(TxInvocationContext ctx, TransactionBoun
      LocalTransaction localTx = (LocalTransaction) ctx.getCacheTransaction();
      LocalizedCacheTopology cacheTopology = dm.getCacheTopology();
      Collection<Address> affectedNodes =
-            isReplicated ? null : cacheTopology.getWriteOwners(getAffectedKeysFromContext(ctx));
+            isReplicated ? null : cacheTopology.getWriteOwners((Collection<Object>) ctx.getAffectedKeys());


you don't need the cast

rvansa · 2017-06-19T08:46:00Z

@pruivo Thanks, rebased & fixed.

rvansa · 2017-06-26T08:50:12Z

Fixed conflict in documentation.

pruivo · 2017-06-28T10:27:21Z

@rvansa and @danberindei is there anything missing?

danberindei · 2017-06-28T12:06:31Z

@pruivo Just one more look...

danberindei · 2017-06-28T14:36:43Z

@pruivo I would have liked to have a longer discussion with @rvansa on MergeOnStore and why it's useful for the atomic maps but not for DeltaAware, but I don't think it would lead to changes to this PR, so I'm fine with integrating.

rvansa · 2017-06-28T15:04:59Z

The purpose of DeltaAware was isolating incremental changes from the changing object on origin, and replicate only Delta. However, it has some issues (e.g. the one addressed by CopyableDeltaAware), sometimes an unclear semantic, and propagation through PKVC required 'extra ifs' as well as custom entry type. All in all it's inferior to use of functional commands, and we don't want to maintain two ways to do the same.

What you wanted to compare is the MergeOnStore - definitely a hack to implement FGAMs which should have never been born :) - and ApplyDeltaCommand which locks on different keys, and who-knows-on what nodes. And applies deltas, as well.

danberindei · 2017-06-28T15:13:56Z

@rvansa Ok, if you say MergeOnStore is a hack that is only ever going to be used by FGAMs I believe you, but you should be aware that it's a public interface in a public package.

rvansa · 2017-06-28T15:21:47Z

@danberindei Once we had to add the check into the code, I made it public since I like extensibility in general. I hope I've defined the behaviour well enough. Maybe it was not a wise choice - if you feel strongly about it, we still have some time to hide the interface. The only thing I'd like to have in mind is that IMO AtomicMap should be moved to its own module in 10.0, rather than staying in core.

wburns · 2017-06-28T15:21:52Z

Sorry this has conflicts now :(

pruivo · 2017-06-28T15:22:06Z

@rvansa needs rebase. @danberindei just press the button when finish the discussion :)

rvansa · 2017-06-28T15:24:21Z

Rebased.

danberindei · 2017-06-28T15:53:46Z

@rvansa Still getting a duplicate externalizer id error on my machine:

18:51:23,137 ERROR (testng-SingleClusterExecutorTest) [TestSuiteProgress] Test failed: org.infinispan.manager.SingleClusterExecutorTest.testExecutorTriConsumer org.infinispan.manager.EmbeddedCacheManagerStartupException: org.infinispan.commons.CacheConfigurationException: ISPN000423: Duplicate id found! AdvancedExternalizer id=122 is shared by another externalizer (org.infinispan.atomic.impl.ApplyDelta$Externalizer)

danberindei · 2017-06-28T16:05:54Z

@rvansa MergeOnStore can't be both a hack and a reasonable public API that should be used by any application at the same time, you have to decide on one :) I vote to make it internal API, because I hope the functional API is going to be good enough to support FGAMs at some point in the future, and I don't want to keep supporting MergeOnStore after that.

* DeltaAware, CopyableDeltaAware, Delta, DeltaCompositeKey deprecated * AdvancedCache.applyDelta deprecated, CacheImpl.applyDelta reimplemented using ReadWriteKeyValueCommand, permits to lock only on the main key * ApplyDeltaCommand and Visitor.visitApplyDeltaCommand deprecated (implementation removed) * DeltaAwareCacheEntry, DeltaAwareObjectOutput and some internal helpers removed

rvansa · 2017-06-29T07:58:17Z

@danberindei I've corrected the Ids and piggy-backed a commit moving MergeOnStore to o.i.container which is (a bit surprisingly) non-public.

danberindei · 2017-06-29T08:27:55Z

@rvansa That is surprising, considering that DataContainer is exposed in the configuration, but I'm fine with it. I'll just wait for another run in CI.

wburns · 2017-06-29T14:18:55Z

Looks like there some related failures http://ci.infinispan.org/job/Infinispan/job/PR-5187/58

wburns · 2017-06-29T14:31:19Z

Actually it looks like this was already failing http://ci.infinispan.org/job/Infinispan/job/master/lastSuccessfulBuild/#showFailuresLink

wburns · 2017-06-29T14:33:57Z

Integrated into master, thanks @rvansa !

rvansa added the Ready for Review label Jun 8, 2017

rvansa requested a review from danberindei June 8, 2017 15:14

rvansa added this to the 9.1.0.Final milestone Jun 8, 2017

rvansa requested a review from wburns June 8, 2017 15:14

rvansa added the Wait CI Results label Jun 8, 2017

tristantarrant added Changes Required and removed Wait CI Results labels Jun 9, 2017

rvansa force-pushed the ISPN-7847 branch from 701fbea to 9657389 Compare June 9, 2017 20:04

rvansa removed the Changes Required label Jun 9, 2017

rvansa force-pushed the ISPN-7847 branch from 9657389 to dd2651e Compare June 9, 2017 20:12

rvansa added the Wait CI Results label Jun 9, 2017

rvansa added Needs Rebase and removed Ready for Review labels Jun 15, 2017

rvansa force-pushed the ISPN-7847 branch from dd2651e to 434cdcf Compare June 16, 2017 08:09

rvansa added Ready for Review and removed Needs Rebase labels Jun 16, 2017

pruivo requested changes Jun 16, 2017

View reviewed changes

pruivo added Changes Required and removed Ready for Review labels Jun 16, 2017

rvansa force-pushed the ISPN-7847 branch from 434cdcf to e63824c Compare June 19, 2017 08:45

rvansa added Ready for Review and removed Changes Required labels Jun 19, 2017

rvansa force-pushed the ISPN-7847 branch from e63824c to 6a2bee9 Compare June 19, 2017 08:49

rvansa force-pushed the ISPN-7847 branch from bff1fe3 to f346913 Compare June 26, 2017 08:49

rvansa force-pushed the ISPN-7847 branch from f346913 to 879ad4b Compare June 28, 2017 11:25

wburns added the Needs Rebase label Jun 28, 2017

rvansa force-pushed the ISPN-7847 branch from 879ad4b to 4a63fa3 Compare June 28, 2017 15:23

rvansa removed the Needs Rebase label Jun 28, 2017

danberindei added Needs Rebase and removed Ready for Review labels Jun 28, 2017

rvansa added 2 commits June 29, 2017 09:52

ISPN-7847 Move MergeOnStore to o.i.container (non-public package)

fb71c92

rvansa force-pushed the ISPN-7847 branch from 4a63fa3 to fb71c92 Compare June 29, 2017 07:56

rvansa added Ready for Review and removed Needs Rebase labels Jun 29, 2017

wburns merged commit ee2d035 into infinispan:master Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISPN-7847 Deprecate DeltaAware #5187

ISPN-7847 Deprecate DeltaAware #5187

rvansa commented Jun 8, 2017

tristantarrant commented Jun 9, 2017

rvansa commented Jun 9, 2017

galderz commented Jun 15, 2017

rvansa commented Jun 15, 2017

rvansa commented Jun 16, 2017

pruivo left a comment

pruivo Jun 16, 2017

rvansa Jun 19, 2017

pruivo Jun 16, 2017

danberindei Jun 19, 2017

pruivo Jun 16, 2017

rvansa commented Jun 19, 2017

rvansa commented Jun 26, 2017

pruivo commented Jun 28, 2017

danberindei commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 28, 2017

wburns commented Jun 28, 2017

pruivo commented Jun 28, 2017

rvansa commented Jun 28, 2017

danberindei commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 29, 2017

danberindei commented Jun 29, 2017

wburns commented Jun 29, 2017

wburns commented Jun 29, 2017

wburns commented Jun 29, 2017

ISPN-7847 Deprecate DeltaAware #5187

ISPN-7847 Deprecate DeltaAware #5187

Conversation

rvansa commented Jun 8, 2017

tristantarrant commented Jun 9, 2017

rvansa commented Jun 9, 2017

galderz commented Jun 15, 2017

rvansa commented Jun 15, 2017

rvansa commented Jun 16, 2017

pruivo left a comment

Choose a reason for hiding this comment

pruivo Jun 16, 2017

Choose a reason for hiding this comment

rvansa Jun 19, 2017

Choose a reason for hiding this comment

pruivo Jun 16, 2017

Choose a reason for hiding this comment

danberindei Jun 19, 2017

Choose a reason for hiding this comment

pruivo Jun 16, 2017

Choose a reason for hiding this comment

rvansa commented Jun 19, 2017

rvansa commented Jun 26, 2017

pruivo commented Jun 28, 2017

danberindei commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 28, 2017

wburns commented Jun 28, 2017

pruivo commented Jun 28, 2017

rvansa commented Jun 28, 2017

danberindei commented Jun 28, 2017

danberindei commented Jun 28, 2017

rvansa commented Jun 29, 2017

danberindei commented Jun 29, 2017

wburns commented Jun 29, 2017

wburns commented Jun 29, 2017

wburns commented Jun 29, 2017