This is an automated email from the ASF dual-hosted git repository.
eshu11 pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push:
new bc77954 Revert "GEODE-5541: A create followed by a destroy of the
same key will not invoke cache listener on remote nodes. (#2282)"
bc77954 is described below
commit bc77954ddce4c1d21dbaef696c3507db983ef242
Author: eshu <[email protected]>
AuthorDate: Wed Aug 15 13:25:09 2018 -0700
Revert "GEODE-5541: A create followed by a destroy of the same key will not
invoke cache listener on remote nodes. (#2282)"
This reverts commit 6f8de005b0026b3e365cb577d07119e0d53c6ed4.
---
...tPartitionedRegionWithTransactionDUnitTest.java | 61 -----------
.../apache/geode/internal/cache/LocalRegion.java | 23 +---
.../apache/geode/internal/cache/TXEntryState.java | 13 ++-
.../geode/internal/cache/LocalRegionTest.java | 118 ---------------------
.../geode/internal/cache/RegionCommitTest.java | 4 +-
5 files changed, 13 insertions(+), 206 deletions(-)
diff --git
a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/partitioned/PersistentPartitionedRegionWithTransactionDUnitTest.java
b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/partitioned/PersistentPartitionedRegionWithTransactionDUnitTest.java
index b30474f..9460e63 100644
---
a/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/partitioned/PersistentPartitionedRegionWithTransactionDUnitTest.java
+++
b/geode-core/src/distributedTest/java/org/apache/geode/internal/cache/partitioned/PersistentPartitionedRegionWithTransactionDUnitTest.java
@@ -15,19 +15,13 @@
package org.apache.geode.internal.cache.partitioned;
import static org.junit.Assert.assertEquals;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.verify;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.apache.geode.cache.Cache;
-import org.apache.geode.cache.CacheListener;
import org.apache.geode.cache.CacheTransactionManager;
import org.apache.geode.cache.Region;
-import org.apache.geode.cache.util.CacheListenerAdapter;
import org.apache.geode.internal.cache.DiskRegion;
import org.apache.geode.internal.cache.DiskStoreImpl;
import org.apache.geode.internal.cache.PartitionedRegion;
@@ -213,59 +207,4 @@ public class
PersistentPartitionedRegionWithTransactionDUnitTest
}
});
}
-
- @Test
- public void NoDestroyInvocationIfCreateEntryAndDestroyItInTransaction()
throws Throwable {
- Host host = Host.getHost(0);
- VM vm0 = host.getVM(0);
- VM vm1 = host.getVM(1);
- VM vm2 = host.getVM(2);
-
- int redundancy = 2;
-
- vm0.invoke(() -> getCacheSetAlwaysFireLocalListeners());
- vm1.invoke(() -> getCacheSetAlwaysFireLocalListeners());
- vm2.invoke(() -> getCacheSetAlwaysFireLocalListeners());
-
- createPR(vm0, redundancy);
- createPR(vm1, redundancy);
- createPR(vm2, redundancy);
-
- vm0.invoke(() -> addListener());
- vm1.invoke(() -> addListener());
- vm2.invoke(() -> addListener());
-
- vm0.invoke(() -> {
- Cache cache = getCache();
- TXManagerImpl txManager = (TXManagerImpl)
cache.getCacheTransactionManager();
- Region region = cache.getRegion(getPartitionedRegionName());
- txManager.begin();
- region.create(1, "toBeDestroyed");
- region.destroy(1);
- txManager.commit();
- });
-
- vm0.invoke(() -> verifyNoDestroyInvocation());
- vm1.invoke(() -> verifyNoDestroyInvocation());
- vm2.invoke(() -> verifyNoDestroyInvocation());
- }
-
- private void getCacheSetAlwaysFireLocalListeners() {
- System.setProperty("gemfire.BucketRegion.alwaysFireLocalListeners",
"true");
- getCache();
- }
-
- private void addListener() {
- Cache cache = getCache();
- Region region = cache.getRegion(getPartitionedRegionName());
- CacheListener listener = spy(new CacheListenerAdapter() {});
- region.getAttributesMutator().addCacheListener(listener);
- }
-
- private void verifyNoDestroyInvocation() throws Exception {
- Cache cache = getCache();
- Region region = cache.getRegion(getPartitionedRegionName());
- CacheListener listener = region.getAttributes().getCacheListeners()[0];
- verify(listener, never()).afterDestroy(any());
- }
}
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index a3609c3..5e2a491 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -6896,10 +6896,7 @@ public class LocalRegion extends AbstractRegion
implements LoaderHelperFactory,
try {
final boolean inTokenMode = needTokensForGII || needRIDestroyToken;
- if (!needsPendingCallbacksForDestroy(key, txEntryState)) {
- pendingCallbacks = new ArrayList<>();
- }
- getRegionMap().txApplyDestroy(key, rmtOrigin, event, inTokenMode,
needRIDestroyToken, op,
+ this.entries.txApplyDestroy(key, rmtOrigin, event, inTokenMode,
needRIDestroyToken, op,
eventId, aCallbackArgument, pendingCallbacks, filterRoutingInfo,
bridgeContext,
isOriginRemote, txEntryState, versionTag, tailKey);
} finally {
@@ -6909,24 +6906,6 @@ public class LocalRegion extends AbstractRegion
implements LoaderHelperFactory,
}
}
- boolean needsPendingCallbacksForDestroy(Object key, TXEntryState
txEntryState) {
- if (isProxy()) {
- return true;
- }
- if (txEntryState != null) {
- // nearside on tx host
- // a destroy on destroyed or removed token, to local committed state,
becomes a noop
- // since nothing needed to be done locally.
- // We don't want to actually do the destroy since we told the
- // transaction listener that no destroy was done.
- Object originalValue = txEntryState.getOriginalValue();
- return originalValue != null && !Token.isRemoved(originalValue);
- }
- // farside without TXEntryState
- RegionEntry regionEntry = basicGetEntry(key);
- return regionEntry != null && !regionEntry.isDestroyedOrRemoved();
- }
-
/**
* Called by lower levels, while still holding the write sync lock, and the
low level has
* completed its part of the basic destroy
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/cache/TXEntryState.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/TXEntryState.java
index d95877d..72a1b2d 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/TXEntryState.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/TXEntryState.java
@@ -19,6 +19,8 @@ import static
org.apache.geode.internal.offheap.annotations.OffHeapIdentifier.TX
import java.io.DataInput;
import java.io.DataOutput;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Set;
import org.apache.logging.log4j.Logger;
@@ -1552,14 +1554,21 @@ public class TXEntryState implements Releasable {
}
private void txApplyDestroyLocally(InternalRegion r, Object key, TXState
txState) {
+ boolean invokeCallbacks = isOpDestroyEvent(r);
+ List<EntryEventImpl> pendingCallbacks =
+ invokeCallbacks ? txState.getPendingCallbacks() : new
ArrayList<EntryEventImpl>();
try {
r.txApplyDestroy(key, txState.getTransactionId(), null, false/*
inTokenMode */,
- getDestroyOperation(), getNearSideEventId(txState), callBackArgument,
- txState.getPendingCallbacks(),
+ getDestroyOperation(), getNearSideEventId(txState),
callBackArgument, pendingCallbacks,
getFilterRoutingInfo(), txState.bridgeContext, false, this, null,
-1);
} catch (RegionDestroyedException ignore) {
} catch (EntryDestroyedException ignore) {
}
+ // if !isOpDestroyEvent then
+ // this destroy, to local committed state, becomes a noop
+ // since nothing needed to be done locally.
+ // We don't want to actually do the destroy since we told the
+ // transaction listener that no destroy was done.
}
private void txApplyInvalidateLocally(InternalRegion r, Object key, Object
newValue,
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
deleted file mode 100644
index 72cc213..0000000
---
a/geode-core/src/test/java/org/apache/geode/internal/cache/LocalRegionTest.java
+++ /dev/null
@@ -1,118 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
- * agreements. See the NOTICE file distributed with this work for additional
information regarding
- * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
- * "License"); you may not use this file except in compliance with the
License. You may obtain a
- * copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
distributed under the License
- * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express
- * or implied. See the License for the specific language governing permissions
and limitations under
- * the License.
- */
-package org.apache.geode.internal.cache;
-
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
-
-import org.junit.Test;
-
-
-public class LocalRegionTest {
-
- private final Object key = new Object();
-
- @Test
- public void proxyRegionNeedsPendingCallbacksForDestroy() {
- TXEntryState txEntryState = mock(TXEntryState.class);
- LocalRegion region = mock(LocalRegion.class);
- when(region.isProxy()).thenReturn(true);
- when(region.needsPendingCallbacksForDestroy(key,
txEntryState)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key,
txEntryState)).isTrue();
- }
-
- @Test
- public void
destroyNullOriginalValueOnTransactionHostDoesNotNeedPendingCallbacksForDestroy()
{
- TXEntryState txEntryState = mock(TXEntryState.class);
- LocalRegion region = mock(LocalRegion.class);
- when(region.isProxy()).thenReturn(false);
- when(txEntryState.getOriginalValue()).thenReturn(null);
- when(region.needsPendingCallbacksForDestroy(key,
txEntryState)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key,
txEntryState)).isFalse();
- }
-
- @Test
- public void
destroyDestroyedTokenOnTransactionHostDoesNotNeedPendingCallbacksForDestroy() {
- TXEntryState txEntryState = mock(TXEntryState.class);
- LocalRegion region = mock(LocalRegion.class);
- when(region.isProxy()).thenReturn(false);
- when(txEntryState.getOriginalValue()).thenReturn(Token.DESTROYED);
- when(region.needsPendingCallbacksForDestroy(key,
txEntryState)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key,
txEntryState)).isFalse();
- }
-
- @Test
- public void
destroyTombstoneOnTransactionHostDoesNotNeedPendingCallbacksForDestroy() {
- TXEntryState txEntryState = mock(TXEntryState.class);
- LocalRegion region = mock(LocalRegion.class);
- when(region.isProxy()).thenReturn(false);
- when(txEntryState.getOriginalValue()).thenReturn(Token.TOMBSTONE);
- when(region.needsPendingCallbacksForDestroy(key,
txEntryState)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key,
txEntryState)).isFalse();
- }
-
- @Test
- public void
destroyNotATokenOnTransactionHostNeedsPendingCallbacksForDestroy() {
- TXEntryState txEntryState = mock(TXEntryState.class);
- LocalRegion region = mock(LocalRegion.class);
- when(region.isProxy()).thenReturn(false);
- when(txEntryState.getOriginalValue()).thenReturn(new Token.NotAToken());
- when(region.needsPendingCallbacksForDestroy(key,
txEntryState)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key,
txEntryState)).isTrue();
- }
-
- @Test
- public void
destroyNonExistingRegionEntryOnRemoteHostDoesNotNeedPendingCallbacksForDestroy()
{
- LocalRegion region = mock(LocalRegion.class);
- when(region.basicGetEntry(key)).thenReturn(null);
- when(region.isProxy()).thenReturn(false);
- when(region.needsPendingCallbacksForDestroy(key,
null)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key, null)).isFalse();
- }
-
- @Test
- public void
destroyRemovedTokenOnRemoteHostDoesNotNeedPendingCallbacksForDestroy() {
- LocalRegion region = mock(LocalRegion.class);
- RegionEntry regionEntry = mock(RegionEntry.class);
- when(region.isProxy()).thenReturn(false);
- when(region.isProxy()).thenReturn(false);
- when(region.basicGetEntry(key)).thenReturn(regionEntry);
- when(regionEntry.isDestroyedOrRemoved()).thenReturn(true);
- when(region.needsPendingCallbacksForDestroy(key,
null)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key, null)).isFalse();
- }
-
- @Test
- public void destroyAValueOnRemoteHostNeedsPendingCallbacksForDestroy() {
- LocalRegion region = mock(LocalRegion.class);
- RegionEntry regionEntry = mock(RegionEntry.class);
- when(region.isProxy()).thenReturn(false);
- when(region.isProxy()).thenReturn(false);
- when(region.basicGetEntry(key)).thenReturn(regionEntry);
- when(regionEntry.isDestroyedOrRemoved()).thenReturn(false);
- when(region.needsPendingCallbacksForDestroy(key,
null)).thenCallRealMethod();
-
- assertThat(region.needsPendingCallbacksForDestroy(key, null)).isTrue();
- }
-}
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/cache/RegionCommitTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/RegionCommitTest.java
index 39218ad..193faf2 100644
---
a/geode-core/src/test/java/org/apache/geode/internal/cache/RegionCommitTest.java
+++
b/geode-core/src/test/java/org/apache/geode/internal/cache/RegionCommitTest.java
@@ -31,8 +31,6 @@ public class RegionCommitTest {
private String path;
private LocalRegion region;
private TXCommitMessage txCommitMessage;
- private RegionCommit regionCommit;
- private final Object key = new Object();
@Before
public void setUp() {
@@ -43,7 +41,6 @@ public class RegionCommitTest {
dm = mock(ClusterDistributionManager.class);
region = mock(LocalRegion.class);
txCommitMessage = mock(TXCommitMessage.class);
- regionCommit = new RegionCommit(txCommitMessage);
when(dm.getCache()).thenReturn(cache);
when(cache.getRegionByPath(path)).thenReturn(region);
@@ -52,6 +49,7 @@ public class RegionCommitTest {
@Test
public void getsRegionFromCacheFromDM() {
+ RegionCommit regionCommit = new RegionCommit(txCommitMessage);
assertThat(regionCommit.getRegionByPath(dm, path)).isEqualTo(region);
}
}
