Skip to content

Commit

Permalink
GEODE-5265: fix dataStoreEntryCount statistic (#2006)
Browse files Browse the repository at this point in the history 
fixed new unit test to not expect updateSizeOnCreate to be called
  • Loading branch information
@dschneider-pivotal
dschneider-pivotal committed Jun 1, 2018
1 parent 7d69f29 commit 9a40c9d
Show file tree
Hide file tree
Showing 5 changed files with 367 additions and 9 deletions.
23 changes: 17 additions & 6 deletions geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,7 @@ public RegionEntry updateRecoveredEntry(Object key, DiskEntry.RecoveredEntry val
try {
if (_isOwnerALocalRegion()) {
boolean oldValueWasTombstone = re.isTombstone();
boolean oldIsDestroyedOrRemoved = re.isDestroyedOrRemoved();
if (oldValueWasTombstone) {
// when a tombstone is to be overwritten, unschedule it first
_getOwner().unscheduleTombstone(re);
Expand All @@ -783,8 +784,10 @@ public RegionEntry updateRecoveredEntry(Object key, DiskEntry.RecoveredEntry val
// value for the cache
if (re.isTombstone()) {
_getOwner().scheduleTombstone(re, re.getVersionStamp().asVersionTag());
_getOwner().updateSizeOnRemove(key, oldSize);
} else if (oldValueWasTombstone) {
if (!oldIsDestroyedOrRemoved) {
_getOwner().updateSizeOnRemove(key, oldSize);
}
} else if (oldIsDestroyedOrRemoved) {
_getOwner().updateSizeOnCreate(key, _getOwner().calculateRegionEntryValueSize(re));
} else {
_getOwner().updateSizeOnPut(key, oldSize,
Expand Down Expand Up @@ -870,6 +873,7 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
}
}
final boolean oldIsTombstone = oldRe.isTombstone();
final boolean oldIsDestroyedOrRemoved = oldRe.isDestroyedOrRemoved();
final int oldSize = owner.calculateRegionEntryValueSize(oldRe);
try {
result = oldRe.initialImagePut(owner, lastModified, newValue, wasRecovered,
Expand All @@ -884,7 +888,9 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
}
}
if (newValue == Token.TOMBSTONE) {
owner.updateSizeOnRemove(key, oldSize);
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
if (owner.getServerProxy() == null
&& owner.getVersionVector().isTombstoneTooOld(
entryVersion.getMemberID(), entryVersion.getRegionVersion())) {
Expand Down Expand Up @@ -1065,6 +1071,7 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent,
if (!re.isRemoved() || re.isTombstone()) {
Object oldValue = re.getValueInVM(owner);
final int oldSize = owner.calculateRegionEntryValueSize(re);
final boolean wasDestroyedOrRemoved = re.isDestroyedOrRemoved();
// Create an entry event only if the calling context is
// a receipt of a TXCommitMessage AND there are callbacks installed
// for this region
Expand Down Expand Up @@ -1112,7 +1119,9 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent,
}
}
EntryLogger.logTXDestroy(_getOwnerObject(), key);
owner.updateSizeOnRemove(key, oldSize);
if (!wasDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
} catch (RegionClearedException rce) {
clearOccured = true;
}
Expand Down Expand Up @@ -1185,6 +1194,7 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent,
}
int oldSize = 0;
boolean wasTombstone = oldRe.isTombstone();
boolean wasDestroyedOrRemoved = oldRe.isDestroyedOrRemoved();
{
if (!wasTombstone) {
oldSize = owner.calculateRegionEntryValueSize(oldRe);
Expand All @@ -1197,7 +1207,9 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent,
if (wasTombstone) {
owner.unscheduleTombstone(oldRe);
}
owner.updateSizeOnRemove(oldRe.getKey(), oldSize);
if (!wasDestroyedOrRemoved) {
owner.updateSizeOnRemove(oldRe.getKey(), oldSize);
}
owner.txApplyDestroyPart2(oldRe, oldRe.getKey(), inTokenMode,
false /* Clear Conflicting with the operation */);
lruEntryDestroy(oldRe);
Expand Down Expand Up @@ -1245,7 +1257,6 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent,
callbackEventAddedToPending = true;
}
EntryLogger.logTXDestroy(_getOwnerObject(), key);
owner.updateSizeOnCreate(newRe.getKey(), 0);
if (owner.getConcurrencyChecksEnabled() && callbackEvent.getVersionTag() != null) {
newRe.makeTombstone(owner, callbackEvent.getVersionTag());
} else if (!inTokenMode) {
Expand Down
5 changes: 4 additions & 1 deletion geode-core/src/main/java/org/apache/geode/internal/cache/map/RegionMapDestroy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,11 +720,14 @@ private boolean destroyEntry(RegionEntry re, EntryEventImpl event, boolean inTok
EntryNotFoundException, RegionClearedException {
focusedRegionMap.processVersionTag(re, event);
final int oldSize = internalRegion.calculateRegionEntryValueSize(re);
final boolean wasRemoved = re.isDestroyedOrRemoved();
boolean retVal = re.destroy(event.getRegion(), event, inTokenMode, cacheWrite, expectedOldValue,
forceDestroy, removeRecoveredEntry);
if (retVal) {
EntryLogger.logDestroy(event);
internalRegion.updateSizeOnRemove(event.getKey(), oldSize);
if (!wasRemoved) {
internalRegion.updateSizeOnRemove(event.getKey(), oldSize);
}
}
return retVal;
}
Expand Down
212 changes: 212 additions & 0 deletions geode-core/src/test/java/org/apache/geode/internal/cache/AbstractRegionMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doThrow;
Expand All @@ -44,13 +45,17 @@
import org.apache.geode.cache.Operation;
import org.apache.geode.cache.Scope;
import org.apache.geode.cache.TransactionId;
import org.apache.geode.cache.client.internal.ServerRegionProxy;
import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
import org.apache.geode.internal.cache.entries.DiskEntry.RecoveredEntry;
import org.apache.geode.internal.cache.eviction.EvictableEntry;
import org.apache.geode.internal.cache.eviction.EvictionController;
import org.apache.geode.internal.cache.eviction.EvictionCounters;
import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
import org.apache.geode.internal.cache.versions.RegionVersionVector;
import org.apache.geode.internal.cache.versions.VersionHolder;
import org.apache.geode.internal.cache.versions.VersionSource;
import org.apache.geode.internal.cache.versions.VersionStamp;
import org.apache.geode.internal.cache.versions.VersionTag;
import org.apache.geode.internal.util.concurrent.ConcurrentMapWithReusableEntries;
import org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap;
Expand Down Expand Up @@ -711,6 +716,196 @@ public void txApplyInvalidateDoesNotInvalidateRemovedToken() throws RegionCleare
}
}

@Test
public void updateRecoveredEntry_givenExistingDestroyedOrRemovedAndSettingToTombstone_neverCallsUpdateSizeOnRemove() {
RecoveredEntry recoveredEntry = mock(RecoveredEntry.class);
RegionEntry regionEntry = mock(RegionEntry.class);
when(regionEntry.isTombstone()).thenReturn(false).thenReturn(true);
when(regionEntry.isDestroyedOrRemoved()).thenReturn(true);
when(regionEntry.getVersionStamp()).thenReturn(mock(VersionStamp.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, regionEntry);

arm.updateRecoveredEntry(KEY, recoveredEntry);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void updateRecoveredEntry_givenExistingRemovedNonTombstone_neverCallsUpdateSizeOnRemove() {
RecoveredEntry recoveredEntry = mock(RecoveredEntry.class);
RegionEntry regionEntry = mock(RegionEntry.class);
when(regionEntry.isRemoved()).thenReturn(true);
when(regionEntry.isTombstone()).thenReturn(false);
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, regionEntry);

arm.updateRecoveredEntry(KEY, recoveredEntry);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void updateRecoveredEntry_givenNoExistingEntry_neverCallsUpdateSizeOnRemove() {
RecoveredEntry recoveredEntry = mock(RecoveredEntry.class);
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, null);

arm.updateRecoveredEntry(KEY, recoveredEntry);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void updateRecoveredEntry_givenExistingNonTombstoneAndSettingToTombstone_callsUpdateSizeOnRemove() {
RecoveredEntry recoveredEntry = mock(RecoveredEntry.class);
RegionEntry regionEntry = mock(RegionEntry.class);
when(regionEntry.isTombstone()).thenReturn(false).thenReturn(true);
when(regionEntry.getVersionStamp()).thenReturn(mock(VersionStamp.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, regionEntry);

arm.updateRecoveredEntry(KEY, recoveredEntry);

verify(arm._getOwner(), times(1)).updateSizeOnRemove(eq(KEY), anyInt());
}

@Test
public void initialImagePut_givenPutIfAbsentReturningDestroyedOrRemovedEntry_neverCallsUpdateSizeOnRemove()
throws RegionClearedException {
ConcurrentMapWithReusableEntries map = mock(ConcurrentMapWithReusableEntries.class);
RegionEntry entry = mock(RegionEntry.class);
when(entry.isDestroyedOrRemoved()).thenReturn(true);
when(entry.initialImagePut(any(), anyLong(), any(), anyBoolean(), anyBoolean()))
.thenReturn(true);
VersionStamp versionStamp = mock(VersionStamp.class);
when(entry.getVersionStamp()).thenReturn(versionStamp);
when(versionStamp.asVersionTag()).thenReturn(mock(VersionTag.class));
when(map.putIfAbsent(eq(KEY), any())).thenReturn(entry).thenReturn(null);
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, map, null);
when(arm._getOwner().getConcurrencyChecksEnabled()).thenReturn(true);
when(arm._getOwner().getServerProxy()).thenReturn(mock(ServerRegionProxy.class));
VersionTag versionTag = mock(VersionTag.class);
when(versionTag.getMemberID()).thenReturn(mock(VersionSource.class));

arm.initialImagePut(KEY, 0, Token.TOMBSTONE, false, false, versionTag, null, false);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void initialImagePut_givenPutIfAbsentReturningNonTombstone_callsUpdateSizeOnRemove()
throws RegionClearedException {
ConcurrentMapWithReusableEntries map = mock(ConcurrentMapWithReusableEntries.class);
RegionEntry entry = mock(RegionEntry.class);
when(entry.isTombstone()).thenReturn(false);
when(entry.isDestroyedOrRemoved()).thenReturn(false);
when(entry.initialImagePut(any(), anyLong(), any(), anyBoolean(), anyBoolean()))
.thenReturn(true);
VersionStamp versionStamp = mock(VersionStamp.class);
when(entry.getVersionStamp()).thenReturn(versionStamp);
when(versionStamp.asVersionTag()).thenReturn(mock(VersionTag.class));
when(map.putIfAbsent(eq(KEY), any())).thenReturn(entry).thenReturn(null);
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, map, null);
when(arm._getOwner().getConcurrencyChecksEnabled()).thenReturn(true);
when(arm._getOwner().getServerProxy()).thenReturn(mock(ServerRegionProxy.class));
VersionTag versionTag = mock(VersionTag.class);
when(versionTag.getMemberID()).thenReturn(mock(VersionSource.class));

arm.initialImagePut(KEY, 0, Token.TOMBSTONE, false, false, versionTag, null, false);

verify(arm._getOwner(), times(1)).updateSizeOnRemove(eq(KEY), anyInt());
}

@Test
public void txApplyDestroy_givenExistingDestroyedOrRemovedEntry_neverCallsUpdateSizeOnRemove() {
RegionEntry regionEntry = mock(RegionEntry.class);
when(regionEntry.isTombstone()).thenReturn(false);
when(regionEntry.isDestroyedOrRemoved()).thenReturn(true);
when(regionEntry.getVersionStamp()).thenReturn(mock(VersionStamp.class));
TXId txId = mock(TXId.class);
when(txId.getMemberId()).thenReturn(mock(InternalDistributedMember.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, regionEntry);

arm.txApplyDestroy(KEY, txId, null, false, false, null, null, null, new ArrayList<>(), null,
null, false, null, null, 0);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void txApplyDestroy_givenExistingNonTombstone_callsUpdateSizeOnRemove() {
RegionEntry regionEntry = mock(RegionEntry.class);
when(regionEntry.isTombstone()).thenReturn(false);
when(regionEntry.getVersionStamp()).thenReturn(mock(VersionStamp.class));
TXId txId = mock(TXId.class);
when(txId.getMemberId()).thenReturn(mock(InternalDistributedMember.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, null, null, regionEntry);

arm.txApplyDestroy(KEY, txId, null, false, false, null, null, null, new ArrayList<>(), null,
null, false, null, null, 0);

verify(arm._getOwner(), times(1)).updateSizeOnRemove(eq(KEY), anyInt());
}

@Test
public void txApplyDestroy_givenPutIfAbsentReturningDestroyedOrRemovedEntry_neverCallsUpdateSizeOnRemove()
throws RegionClearedException {
ConcurrentMapWithReusableEntries map = mock(ConcurrentMapWithReusableEntries.class);
RegionEntry entry = mock(RegionEntry.class);
when(entry.isTombstone()).thenReturn(false);
when(entry.isDestroyedOrRemoved()).thenReturn(true);
VersionStamp versionStamp = mock(VersionStamp.class);
when(entry.getVersionStamp()).thenReturn(versionStamp);
when(versionStamp.asVersionTag()).thenReturn(mock(VersionTag.class));
when(map.putIfAbsent(eq(KEY), any())).thenReturn(entry).thenReturn(null);
TXId txId = mock(TXId.class);
when(txId.getMemberId()).thenReturn(mock(InternalDistributedMember.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, map, null);
when(arm._getOwner().getConcurrencyChecksEnabled()).thenReturn(true);

arm.txApplyDestroy(KEY, txId, null, false, false, null, null, null, new ArrayList<>(), null,
null, false, null, null, 0);

verify(arm._getOwner(), never()).updateSizeOnRemove(any(), anyInt());
}

@Test
public void txApplyDestroy_givenPutIfAbsentReturningNonTombstone_callsUpdateSizeOnRemove()
throws RegionClearedException {
ConcurrentMapWithReusableEntries map = mock(ConcurrentMapWithReusableEntries.class);
RegionEntry entry = mock(RegionEntry.class);
when(entry.getKey()).thenReturn(KEY);
when(entry.isTombstone()).thenReturn(false);
VersionStamp versionStamp = mock(VersionStamp.class);
when(entry.getVersionStamp()).thenReturn(versionStamp);
when(versionStamp.asVersionTag()).thenReturn(mock(VersionTag.class));
when(map.putIfAbsent(eq(KEY), any())).thenReturn(entry).thenReturn(null);
TXId txId = mock(TXId.class);
when(txId.getMemberId()).thenReturn(mock(InternalDistributedMember.class));
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, map, null);
when(arm._getOwner().getConcurrencyChecksEnabled()).thenReturn(true);

arm.txApplyDestroy(KEY, txId, null, false, false, null, null, null, new ArrayList<>(), null,
null, false, null, null, 0);

verify(arm._getOwner(), times(1)).updateSizeOnRemove(eq(KEY), anyInt());
}

@Test
public void txApplyDestroy_givenFactory_neverCallsUpdateSizeOnRemove()
throws RegionClearedException {
ConcurrentMapWithReusableEntries map = mock(ConcurrentMapWithReusableEntries.class);
RegionEntry entry = mock(RegionEntry.class);
TXId txId = mock(TXId.class);
when(txId.getMemberId()).thenReturn(mock(InternalDistributedMember.class));
RegionEntryFactory factory = mock(RegionEntryFactory.class);
when(factory.createEntry(any(), any(), any())).thenReturn(entry);
TestableAbstractRegionMap arm = new TestableAbstractRegionMap(false, map, factory);
when(arm._getOwner().getConcurrencyChecksEnabled()).thenReturn(true);

arm.txApplyDestroy(KEY, txId, null, false, false, null, null, null, new ArrayList<>(), null,
null, false, null, null, 0);

verify(arm._getOwner(), never()).updateSizeOnCreate(any(), anyInt());
}

private EntryEventImpl createEventForInvalidate(LocalRegion lr) {
when(lr.getKeyInfo(KEY)).thenReturn(new KeyInfo(KEY, null, null));
return EntryEventImpl.create(lr, Operation.INVALIDATE, KEY, false, null, true, false);
Expand Down Expand Up @@ -743,6 +938,7 @@ private void verifyTxApplyInvalidate(TxTestableAbstractRegionMap arm, Object key
* TestableAbstractRegionMap
*/
private static class TestableAbstractRegionMap extends AbstractRegionMap {
private final RegionEntry regionEntryForGetEntry;

protected TestableAbstractRegionMap() {
this(false);
Expand All @@ -754,7 +950,14 @@ protected TestableAbstractRegionMap(boolean withConcurrencyChecks) {

protected TestableAbstractRegionMap(boolean withConcurrencyChecks,
ConcurrentMapWithReusableEntries map, RegionEntryFactory factory) {
this(withConcurrencyChecks, map, factory, null);
}

protected TestableAbstractRegionMap(boolean withConcurrencyChecks,
ConcurrentMapWithReusableEntries map, RegionEntryFactory factory,
RegionEntry regionEntryForGetEntry) {
super(null);
this.regionEntryForGetEntry = regionEntryForGetEntry;
LocalRegion owner = mock(LocalRegion.class);
CachePerfStats cachePerfStats = mock(CachePerfStats.class);
when(owner.getCachePerfStats()).thenReturn(cachePerfStats);
Expand All @@ -771,6 +974,15 @@ protected TestableAbstractRegionMap(boolean withConcurrencyChecks,
setEntryFactory(factory);
}
}

@Override
public RegionEntry getEntry(Object key) {
if (this.regionEntryForGetEntry != null) {
return this.regionEntryForGetEntry;
} else {
return super.getEntry(key);
}
}
}


Expand Down
5 changes: 3 additions & 2 deletions ...re/src/test/java/org/apache/geode/internal/cache/AbstractRegionMapTxApplyDestroyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.assertj.core.api.SoftAssertions.assertSoftly;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.inOrder;
Expand Down Expand Up @@ -861,14 +862,14 @@ public void txApplyDestroySetsRegionOnEvent_givenFactoryRegionEntryAndBucket() {
}

@Test
public void txApplyDestroyCallUpdateSizeOnCreate_givenFactoryRegionEntry() {
public void txApplyDestroyNeverCallsUpdateSizeOnCreate_givenFactoryRegionEntry() {
givenLocalRegion();
givenConcurrencyChecks();
givenFactoryRegionEntry();

doTxApplyDestroy();

verify(owner, times(1)).updateSizeOnCreate(eq(key), eq(0));
verify(owner, never()).updateSizeOnCreate(any(), anyInt());
}

@Test
Expand Down
Loading

0 comments on commit 9a40c9d

Please sign in to comment.