This is an automated email from the ASF dual-hosted git repository. zhouxj pushed a commit to branch feature/GEODE-5908 in repository https://gitbox.apache.org/repos/asf/geode.git
commit e0bc9ce1b3cb6073025e74aaf5247b861dc5747e Author: zhouxh <[email protected]> AuthorDate: Sat Oct 20 22:47:32 2018 -0700 GEODE-5908: DiskStoreID.compare should compare mostSig, then leastSig --- .../internal/cache/persistence/DiskStoreID.java | 2 +- .../cache/entries/AbstractRegionEntryTest.java | 86 +++++++++++++++++++--- 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskStoreID.java b/geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskStoreID.java index 601d248..0392e92 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskStoreID.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/persistence/DiskStoreID.java @@ -80,7 +80,7 @@ public class DiskStoreID implements VersionSource<DiskStoreID>, Serializable { return 1; } int result = Long.signum(mostSig - tagID.mostSig); - if (result != 0) { + if (result == 0) { result = Long.signum(leastSig - tagID.leastSig); } return result; diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/entries/AbstractRegionEntryTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/entries/AbstractRegionEntryTest.java index 21ed680..fc5882d 100644 --- a/geode-core/src/test/java/org/apache/geode/internal/cache/entries/AbstractRegionEntryTest.java +++ b/geode-core/src/test/java/org/apache/geode/internal/cache/entries/AbstractRegionEntryTest.java @@ -14,10 +14,13 @@ */ package org.apache.geode.internal.cache.entries; +import static org.apache.geode.internal.Assert.assertTrue; +import static org.apache.geode.internal.Assert.fail; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -36,6 +39,7 @@ import org.apache.geode.cache.query.QueryException; import org.apache.geode.cache.query.internal.index.IndexManager; import org.apache.geode.cache.query.internal.index.IndexProtocol; import org.apache.geode.distributed.internal.membership.InternalDistributedMember; +import org.apache.geode.internal.cache.CachePerfStats; import org.apache.geode.internal.cache.EntryEventImpl; import org.apache.geode.internal.cache.GemFireCacheImpl; import org.apache.geode.internal.cache.InternalRegion; @@ -43,6 +47,7 @@ import org.apache.geode.internal.cache.LocalRegion; import org.apache.geode.internal.cache.RegionClearedException; import org.apache.geode.internal.cache.RegionEntryContext; import org.apache.geode.internal.cache.Token; +import org.apache.geode.internal.cache.persistence.DiskStoreID; import org.apache.geode.internal.cache.versions.ConcurrentCacheModificationException; import org.apache.geode.internal.cache.versions.RegionVersionVector; import org.apache.geode.internal.cache.versions.VersionSource; @@ -186,32 +191,89 @@ public class AbstractRegionEntryTest { re.processVersionTag(entryEvent1); } + @Test + public void shouldCompareDiskStoreIdInCorrectOrder() { + // create 2 tag with save version, differen DiskStoreID, + // apply one tag to stamp + // then compare stamp's DiskStoreID with another tag's + LocalRegion lr = mock(LocalRegion.class); + String value = "value"; + AbstractRegionEntry re = new TestableRegionEntry(lr, value); + InternalDistributedMember member1 = mock(InternalDistributedMember.class); + + // note: diskStoreID_1 > diskStoreID_2 if considered mostSig + // Note: the current code will treat diskStoreID_1 == diskStoreID_3 + DiskStoreID diskStoreID_1 = new DiskStoreID(0xa870e3a0126e4b0dL, 0x87559e428bd3ad67L); + DiskStoreID diskStoreID_2 = new DiskStoreID(0x5a42b9ec2e6e45dbL, 0x9a3c0f29623d925cL); + DiskStoreID diskStoreID_3 = new DiskStoreID(0x5a42b9ec2e6e45dbL, 0x87559e428bd3ad67L); + assertTrue(diskStoreID_1.compareTo(diskStoreID_2) > 0); // should + assertTrue(diskStoreID_1.compareTo(diskStoreID_3) > 0); // should + + RegionVersionVector rvv = mock(RegionVersionVector.class); + CachePerfStats stats = mock(CachePerfStats.class); + when(lr.getVersionVector()).thenReturn(rvv); + when(rvv.getCanonicalId(any())).thenReturn(diskStoreID_2); + when(lr.getCachePerfStats()).thenReturn(stats); + doNothing().when(stats).incConflatedEventsCount(); + + // create tag1, tag2, tag3 with different diskstoreId + VersionTag tag1 = VersionTag.create(diskStoreID_1); + tag1.setEntryVersion(5); // set the same entry version to force it compare DiskStoreID + tag1.setVersionTimeStamp(2); + tag1.setDistributedSystemId(2); + tag1.setIsGatewayTag(false); + tag1.setPosDup(true); + + VersionTag tag2 = VersionTag.create(diskStoreID_2); + tag2.setEntryVersion(5); // set the same entry version to force it compare DiskStoreID + tag2.setVersionTimeStamp(2); + tag2.setDistributedSystemId(2); + tag2.setIsGatewayTag(false); + + VersionTag tag3 = VersionTag.create(diskStoreID_3); + tag3.setEntryVersion(5); // set the same entry version to force it compare DiskStoreID + tag3.setVersionTimeStamp(2); + tag3.setDistributedSystemId(2); + tag3.setIsGatewayTag(false); + + // assign tag1 into stamp + ((TestableRegionEntry) re).setVersions(tag1); + + try { + re.basicProcessVersionTag(lr, tag2, false, false, diskStoreID_2, + member1, true); + fail("expect CME here"); + } catch (ConcurrentCacheModificationException cme) { + assertEquals(null, cme.getMessage()); + } + + // tag3 in stamp should accept tag1's apply, because tag1 is actually bigger + // but since leastsig is the same, tag1 and tag3 was treated as equal + ((TestableRegionEntry) re).setVersions(tag3); + re.basicProcessVersionTag(lr, tag1, false, false, diskStoreID_1, + member1, true); + } + public static class TestableRegionEntry extends AbstractRegionEntry implements OffHeapRegionEntry, VersionStamp { private Object value; private VersionTag tag; - private long timeStamp = 0; protected TestableRegionEntry(RegionEntryContext context, Object value) { super(context, value); } @Override - public void setVersionTimeStamp(long timeStamp) { - this.timeStamp = timeStamp; - } + public void setVersionTimeStamp(long timeStamp) {} @Override public void setVersions(VersionTag tag) { this.tag = tag; - this.timeStamp = tag.getVersionTimeStamp(); } @Override - public void setMemberID(VersionSource memberID) { - - } + public void setMemberID(VersionSource memberID) {} @Override public VersionTag asVersionTag() { @@ -297,22 +359,22 @@ public class AbstractRegionEntryTest { @Override public int getEntryVersion() { - return 0; + return this.tag.getEntryVersion(); } @Override public long getRegionVersion() { - return 0; + return this.tag.getRegionVersion(); } @Override public long getVersionTimeStamp() { - return this.timeStamp; + return this.tag.getVersionTimeStamp(); } @Override public VersionSource getMemberID() { - return null; + return this.tag.getMemberID(); } @Override
