You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/12/16 23:20:56 UTC
[GitHub] [geode] boglesby opened a new pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
boglesby opened a new pull request #5859:
URL: https://github.com/apache/geode/pull/5859
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] albertogpz commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
albertogpz commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r545234359
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -10219,6 +10219,7 @@ public void initializeStats(long numEntriesInVM, long numOverflowOnDisk,
long numOverflowBytesOnDisk) {
getDiskRegion().getStats().incNumEntriesInVM(numEntriesInVM);
getDiskRegion().getStats().incNumOverflowOnDisk(numOverflowOnDisk);
+ getDiskRegion().getStats().incNumOverflowBytesOnDisk(numOverflowBytesOnDisk);
Review comment:
I would put this change in a different PR/JIRA as it is about a different bug.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,11 +838,11 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
Review comment:
How about putting the `lruEntryUpdate(oldRe)` and `lruEntryCreate(oldRe)`calls inside the `else` for the case in which the value for the new entry is not a tombstone as follows:
```
...
} else { //newValue != Token.TOMBSTONE
int newSize = owner.calculateRegionEntryValueSize(oldRe);
if (!oldIsTombstone) {
lruEntryUpdate(oldRe);
owner.updateSizeOnPut(key, oldSize, newSize);
} else {
lruEntryCreate(oldRe);
owner.updateSizeOnCreate(key, newSize);
}
...
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby merged pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby merged pull request #5859:
URL: https://github.com/apache/geode/pull/5859
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r556709683
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,24 +838,23 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
- lruEntryCreate(oldRe);
- } else {
- lruEntryUpdate(oldRe);
- }
}
if (newValue == Token.TOMBSTONE) {
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
owner.scheduleTombstone(oldRe, entryVersion);
- lruEntryDestroy(oldRe);
+ if (!oldIsTombstone) {
Review comment:
Yes. That was part of the problem. The code used to do this (abbreviated):
```
if (oldIsTombstone) {
if (newValue != Token.TOMBSTONE) {
lruEntryCreate(oldRe);
} else {
lruEntryUpdate(oldRe);
}
}
if (newValue == Token.TOMBSTONE) {
lruEntryDestroy(oldRe);
}
```
So if `oldIsTombstone` and `newValue == Token.TOMBSTONE`, both `lruEntryUpdate and `lruEntryDestroy` were called.
And, `lruEntryCreate` \ `lruEntryUpdate` were not called at all when `newValue != Token.TOMBSTONE`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r553508185
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,11 +838,11 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
Review comment:
Great. Thanks a lot @dschneider-pivotal. I'll make the changes you suggest.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r545298436
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -10219,6 +10219,7 @@ public void initializeStats(long numEntriesInVM, long numOverflowOnDisk,
long numOverflowBytesOnDisk) {
getDiskRegion().getStats().incNumEntriesInVM(numEntriesInVM);
getDiskRegion().getStats().incNumOverflowOnDisk(numOverflowOnDisk);
+ getDiskRegion().getStats().incNumOverflowBytesOnDisk(numOverflowBytesOnDisk);
Review comment:
Thanks for the comment. I'll definitely move this change to the correct JIRA.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r553500739
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,11 +838,11 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
Review comment:
I agree with Alberto. I don't see any reason to do call lruEntryUpdate/lruEntryCreate if the next thing we are going to do is call lruEntryDestroy. But it does seem like this code should make sure to call one of lruEntry(Create/Update/Destroy). So I would change it to be:
```
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
}
if (newValue == Token.TOMBSTONE) {
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
owner.scheduleTombstone(oldRe, entryVersion);
lruEntryDestroy(oldRe);
} else {
int newSize = owner.calculateRegionEntryValueSize(oldRe);
if (!oldIsTombstone) {
owner.updateSizeOnPut(key, oldSize, newSize);
lruEntryUpdate(oldRe);
} else {
owner.updateSizeOnCreate(key, newSize);
lruEntryCreate(oldRe);
}
EntryLogger.logInitialImagePut(_getOwnerObject(), key, newValue);
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r553500739
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,11 +838,11 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
Review comment:
I agree with Alberto. I don't see any reason to do call lruEntryUpdate/lruEntryCreate if the next thing we are going to do is call lruEntryDestroy. But it does seem like this code should make sure to call one of lruEntry(Create/Update/Destroy). So I would change it to be:
` if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
}
if (newValue == Token.TOMBSTONE) {
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
owner.scheduleTombstone(oldRe, entryVersion);
lruEntryDestroy(oldRe);
} else {
int newSize = owner.calculateRegionEntryValueSize(oldRe);
if (!oldIsTombstone) {
owner.updateSizeOnPut(key, oldSize, newSize);
lruEntryUpdate(oldRe);
} else {
owner.updateSizeOnCreate(key, newSize);
lruEntryCreate(oldRe);
}
EntryLogger.logInitialImagePut(_getOwnerObject(), key, newValue);
}
}
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r556709683
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,24 +838,23 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
- lruEntryCreate(oldRe);
- } else {
- lruEntryUpdate(oldRe);
- }
}
if (newValue == Token.TOMBSTONE) {
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
owner.scheduleTombstone(oldRe, entryVersion);
- lruEntryDestroy(oldRe);
+ if (!oldIsTombstone) {
Review comment:
Yes. That was part of the problem. The code used to do this (abbreviated):
```
if (oldIsTombstone) {
if (newValue != Token.TOMBSTONE) {
lruEntryCreate(oldRe);
} else {
lruEntryUpdate(oldRe);
}
}
if (newValue == Token.TOMBSTONE) {
lruEntryDestroy(oldRe);
}
```
So if `oldIsTombstone` and `newValue == Token.TOMBSTONE`, both `lruEntryUpdate` and `lruEntryDestroy` were called.
And, `lruEntryCreate` \ `lruEntryUpdate` were not called at all when `newValue != Token.TOMBSTONE`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r556125998
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,24 +838,23 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
- lruEntryCreate(oldRe);
- } else {
- lruEntryUpdate(oldRe);
- }
}
if (newValue == Token.TOMBSTONE) {
if (!oldIsDestroyedOrRemoved) {
owner.updateSizeOnRemove(key, oldSize);
}
owner.scheduleTombstone(oldRe, entryVersion);
- lruEntryDestroy(oldRe);
+ if (!oldIsTombstone) {
Review comment:
One question I have here is, when `newValue == Token.TOMBSTONE` and `oldIsTombstone == true`, it used to have `lruEntryUpdate(oldRe)`, which is in the deleted line 844. Now, after the code change, it is not having `lruEntryUpdate(oldRe)`, when `newValue == Token.TOMBSTONE` and `oldIsTombstone == true`. Is this expected?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] boglesby commented on a change in pull request #5859: GEODE-8278: Modified to cause lruEntryUpdate to be called
Posted by GitBox <gi...@apache.org>.
boglesby commented on a change in pull request #5859:
URL: https://github.com/apache/geode/pull/5859#discussion_r545297897
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
##########
@@ -838,11 +838,11 @@ public boolean initialImagePut(final Object key, final long lastModified, Object
if (result) {
if (oldIsTombstone) {
owner.unscheduleTombstone(oldRe);
- if (newValue != Token.TOMBSTONE) {
Review comment:
Thanks for taking a look at these changes. I mainly put this PR up to see if it would pass the unit tests. This was the change that I was concerned about. Event though the tests passed, I'm still not sure its a valid change. I have a request to another team to take a look at these changes before I proceed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org