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 877bcc6 GEODE-5748: Hold clearRegion write lock during
cleanUpAfterFailedGII (#2486)
877bcc6 is described below
commit 877bcc666137a51abce2e3a8edda36ab97db4e23
Author: pivotal-eshu <[email protected]>
AuthorDate: Tue Sep 18 11:37:45 2018 -0700
GEODE-5748: Hold clearRegion write lock during cleanUpAfterFailedGII (#2486)
* To avoid race with concurrent cache operation, a clearRegion write lock
is needed
when clearing entries and disk region entries during
cleanUpAfterFailedGII, as cache
operations hold clearRegion read lock.
---
.../geode/internal/cache/DistributedRegion.java | 40 ++++++++++++--------
.../internal/cache/DistributedRegionTest.java | 44 ++++++++++++++++++++++
2 files changed, 69 insertions(+), 15 deletions(-)
diff --git
a/geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java
b/geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java
index 42dcd98..488b1fa 100755
---
a/geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java
+++
b/geode-core/src/main/java/org/apache/geode/internal/cache/DistributedRegion.java
@@ -1331,23 +1331,33 @@ public class DistributedRegion extends LocalRegion
implements InternalDistribute
return;
}
- if (!this.entries.isEmpty()) {
- closeEntries();
- if (getDiskRegion() != null) {
- getDiskRegion().clear(this, null);
- }
- // clear the left-members and version-tags sets in imageState
- getImageState().getLeftMembers();
- getImageState().getVersionTags();
- // Clear OQL indexes
- if (this.indexManager != null) {
- try {
- this.indexManager.rerunIndexCreationQuery();
- } catch (Exception ex) {
- if (logger.isDebugEnabled()) {
- logger.debug("Exception while clearing indexes after GII
failure.", ex);
+ if (!getRegionMap().isEmpty()) {
+ RegionVersionVector rvv = getVersionVector();
+ if (rvv != null) {
+ rvv.lockForClear(getFullPath(), getDistributionManager(), getMyId());
+ }
+ try {
+ closeEntries();
+ if (getDiskRegion() != null) {
+ getDiskRegion().clear(this, null);
+ }
+ // clear the left-members and version-tags sets in imageState
+ getImageState().getLeftMembers();
+ getImageState().getVersionTags();
+ // Clear OQL indexes
+ if (this.indexManager != null) {
+ try {
+ this.indexManager.rerunIndexCreationQuery();
+ } catch (Exception ex) {
+ if (logger.isDebugEnabled()) {
+ logger.debug("Exception while clearing indexes after GII
failure.", ex);
+ }
}
}
+ } finally {
+ if (rvv != null) {
+ rvv.unlockForClear(getMyId());
+ }
}
}
}
diff --git
a/geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java
b/geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java
index 731082b..cf27546 100644
---
a/geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java
+++
b/geode-core/src/test/java/org/apache/geode/internal/cache/DistributedRegionTest.java
@@ -15,13 +15,21 @@
package org.apache.geode.internal.cache;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Matchers.anyObject;
import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.doCallRealMethod;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import org.junit.Test;
+import
org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.versions.RegionVersionVector;
+
public class DistributedRegionTest {
@@ -37,4 +45,40 @@ public class DistributedRegionTest {
assertThat(mockDistributedRegion.validatedDestroy(new Object(),
mockEntryEventImpl))
.isSameAs(returnValue);
}
+
+ @Test
+ public void cleanUpAfterFailedGIIHoldsLockForClear() {
+ DistributedRegion distributedRegion = mock(DistributedRegion.class,
RETURNS_DEEP_STUBS);
+ RegionVersionVector regionVersionVector = mock(RegionVersionVector.class);
+ RegionMap regionMap = mock(RegionMap.class);
+ InternalDistributedMember member = mock(InternalDistributedMember.class);
+
+ doCallRealMethod().when(distributedRegion).cleanUpAfterFailedGII(false);
+ when(distributedRegion.getVersionVector()).thenReturn(regionVersionVector);
+ when(distributedRegion.getRegionMap()).thenReturn(regionMap);
+ when(regionMap.isEmpty()).thenReturn(false);
+ when(distributedRegion.getMyId()).thenReturn(member);
+
+ distributedRegion.cleanUpAfterFailedGII(false);
+
+ verify(regionVersionVector).lockForClear(any(), any(), eq(member));
+ verify(distributedRegion).closeEntries();
+ verify(regionVersionVector).unlockForClear(eq(member));
+ }
+
+ @Test
+ public void
cleanUpAfterFailedGIIDoesNotCloseEntriesIfIsPersistentRegionAndRecoveredFromDisk()
{
+ DistributedRegion distributedRegion = mock(DistributedRegion.class);
+ DiskRegion diskRegion = mock(DiskRegion.class);
+
+ doCallRealMethod().when(distributedRegion).cleanUpAfterFailedGII(true);
+ when(distributedRegion.getDiskRegion()).thenReturn(diskRegion);
+ when(diskRegion.isBackup()).thenReturn(true);
+
+ distributedRegion.cleanUpAfterFailedGII(true);
+
+
+ verify(diskRegion).resetRecoveredEntries(eq(distributedRegion));
+ verify(distributedRegion, never()).closeEntries();
+ }
}
