[geode] branch feature/GEODE-6261 updated: GEODE-6261: Do not break repeatable read by incorrectly cleaning up TXEntries

[es...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

eshu11 pushed a commit to branch feature/GEODE-6261
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/feature/GEODE-6261 by this push:
     new 309bd7b  GEODE-6261: Do not break repeatable read by incorrectly cleaning up TXEntries
309bd7b is described below

commit 309bd7b6bc4a1b111ee9b5cbbb189edc1da1f138
Author: eshu <es...@pivotal.io>
AuthorDate: Wed Jan 9 15:27:37 2019 -0800

    GEODE-6261: Do not break repeatable read by incorrectly cleaning up TXEntries
    
     * Do not cleanup existing TXEntries if finding an entry does not have expected value.
---
 .../java/org/apache/geode/TXJUnitTest.java         | 47 ++++++++++++++++++++++
 .../org/apache/geode/internal/cache/TXState.java   |  1 -
 .../apache/geode/internal/cache/TXStateTest.java   | 28 ++++++++++++-
 3 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/geode-core/src/integrationTest/java/org/apache/geode/TXJUnitTest.java b/geode-core/src/integrationTest/java/org/apache/geode/TXJUnitTest.java
index 408cc6b..545baf2 100644
--- a/geode-core/src/integrationTest/java/org/apache/geode/TXJUnitTest.java
+++ b/geode-core/src/integrationTest/java/org/apache/geode/TXJUnitTest.java
@@ -70,6 +70,8 @@ import org.apache.geode.cache.PartitionAttributes;
 import org.apache.geode.cache.PartitionAttributesFactory;
 import org.apache.geode.cache.Region;
 import org.apache.geode.cache.RegionEvent;
+import org.apache.geode.cache.RegionFactory;
+import org.apache.geode.cache.RegionShortcut;
 import org.apache.geode.cache.Scope;
 import org.apache.geode.cache.TimeoutException;
 import org.apache.geode.cache.TransactionEvent;
@@ -7208,4 +7210,49 @@ public class TXJUnitTest {
     assertTrue(!ev.getOperation().isNetSearch());
   }
 
+  private enum OperationType {
+    REPLACE, REMOVE
+  }
+
+  @Test
+  public void removeShouldNotCleanupRepeatableReadTXEntriesIfEntryNotFound() {
+    repeatableReadTXEntriesShouldNotBeCleanedUpIfEntryNotFound(OperationType.REMOVE);
+  }
+
+  @Test
+  public void replaceShouldNotCleanupRepeatableReadTXEntriesIfEntryNotFound() {
+    repeatableReadTXEntriesShouldNotBeCleanedUpIfEntryNotFound(OperationType.REPLACE);
+  }
+
+  private void repeatableReadTXEntriesShouldNotBeCleanedUpIfEntryNotFound(OperationType type) {
+    RegionFactory regionFactory = cache.createRegionFactory(RegionShortcut.REPLICATE);
+    Region<Integer, String> region = regionFactory.create(getUniqueName());
+    region.put(1, "value1");
+    region.put(2, "value2");
+    txMgr.begin();
+    region.put(1, "newValue1");
+    TransactionId transaction1 = txMgr.suspend();
+
+    txMgr.begin();
+    region.get(1); // a repeatable read operation
+    switch (type) {
+      case REMOVE:
+        assertThat(region.remove(2, "nonExistingValue")).isFalse();
+        break;
+      case REPLACE:
+        assertThat(region.replace(2, "newValue", "nonExistingValue")).isFalse();
+        break;
+      default:
+        throw new RuntimeException("Unknown operation");
+    };
+    TransactionId transaction2 = txMgr.suspend();
+
+    txMgr.resume(transaction1);
+    txMgr.commit();
+
+    txMgr.resume(transaction2);
+    region.put(1, "anotherValue");
+    assertThatThrownBy(() -> txMgr.commit()).isInstanceOf(CommitConflictException.class);
+  }
+
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java b/geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java
index 3474a2d..2af3146 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java
@@ -1553,7 +1553,6 @@ public class TXState implements TXStateInterface {
       if (expectedOldValue != null) {
         Object val = result.getNearSidePendingValue();
         if (!AbstractRegionEntry.checkExpectedOldValue(expectedOldValue, val, internalRegion)) {
-          txr.cleanupNonDirtyEntries(internalRegion);
           throw new EntryNotFoundException(
               "The current value was not equal to expected value.");
         }
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
index 2727ec9..fad1973 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
@@ -34,7 +34,9 @@ import org.junit.Before;
 import org.junit.Test;
 
 import org.apache.geode.cache.CommitConflictException;
+import org.apache.geode.cache.EntryNotFoundException;
 import org.apache.geode.cache.FailedSynchronizationException;
+import org.apache.geode.cache.RegionAttributes;
 import org.apache.geode.cache.SynchronizationCommitConflictException;
 import org.apache.geode.cache.TransactionDataNodeHasDepartedException;
 import org.apache.geode.cache.TransactionException;
@@ -180,7 +182,7 @@ public class TXStateTest {
   }
 
   @Test
-  public void txReadEntryDoesNotCleanupNonDirtyEntriesIfRegionCreateReadEntryReturnsNull() {
+  public void txReadEntryDoesNotCleanupTXEntriesIfRegionCreateReadEntryReturnsNull() {
     TXState txState = spy(new TXState(txStateProxy, true));
     KeyInfo keyInfo = mock(KeyInfo.class);
     Object key = new Object();
@@ -197,4 +199,28 @@ public class TXStateTest {
     verify(txRegionState, never()).cleanupNonDirtyEntries(dataRegion);
   }
 
+  @Test
+  public void txReadEntryDoesNotCleanupTXEntriesIfEntryNotFound() {
+    TXState txState = spy(new TXState(txStateProxy, true));
+    KeyInfo keyInfo = mock(KeyInfo.class);
+    Object key = new Object();
+    Object expectedValue = new Object();
+    InternalRegion internalRegion = mock(InternalRegion.class);
+    InternalRegion dataRegion = mock(InternalRegion.class);
+    TXRegionState txRegionState = mock(TXRegionState.class);
+    TXEntryState txEntryState = mock(TXEntryState.class);
+    when(internalRegion.getDataRegionForWrite(keyInfo)).thenReturn(dataRegion);
+    when(internalRegion.getAttributes()).thenReturn(mock(RegionAttributes.class));
+    when(internalRegion.getCache()).thenReturn(mock(InternalCache.class));
+    when(txState.txReadRegion(dataRegion)).thenReturn(txRegionState);
+    when(keyInfo.getKey()).thenReturn(key);
+    when(txRegionState.readEntry(key)).thenReturn(txEntryState);
+    when(txEntryState.getNearSidePendingValue()).thenReturn("currentVal");
+
+    assertThatThrownBy(
+        () -> txState.txReadEntry(keyInfo, internalRegion, true, expectedValue, false))
+            .isInstanceOf(EntryNotFoundException.class);
+    verify(txRegionState, never()).cleanupNonDirtyEntries(internalRegion);
+  }
+
 }