-
Notifications
You must be signed in to change notification settings - Fork 684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GEODE-5265: fix dataStoreEntryCount statistic #2006
GEODE-5265: fix dataStoreEntryCount statistic #2006
Conversation
@@ -884,7 +886,9 @@ public boolean initialImagePut(final Object key, final long lastModified, Object | |||
} | |||
} | |||
if (newValue == Token.TOMBSTONE) { | |||
owner.updateSizeOnRemove(key, oldSize); | |||
if (!oldIsTombstone) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this should care about old being "isRemoved()" instead of only "isTombstone()".
@@ -1065,6 +1069,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 wasTombstone = re.isTombstone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isTombstone is not enough. Token.DESTROYED should also be handled.
@@ -1196,8 +1203,9 @@ public void txApplyDestroy(Object key, TransactionId txId, TXRmtEvent txEvent, | |||
EntryLogger.logTXDestroy(_getOwnerObject(), key); | |||
if (wasTombstone) { | |||
owner.unscheduleTombstone(oldRe); | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code should also handle Token.DESTROY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This reverts commit 44e5d36.
fixed new unit test to not expect updateSizeOnCreate to be called
A number of places existed in which even though the entry had already been destroyed; it could be destroyed again. In those cases the statistic would decremented an extra time.
In one place the stat was incremented when doing txApplyDestroy even though an entry was not being added to the region.
Unit tests were added for all these places that failed before the code was fixed.
@dhemery