dschneider-pivotal commented on a change in pull request #7448:
URL: https://github.com/apache/geode/pull/7448#discussion_r830149463
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -2109,24 +2109,26 @@ int entryCount(Set<Integer> buckets, boolean estimate) {
@Override
public int getRegionSize() {
synchronized (getSizeGuard()) {
- int result = getRegionMap().size();
- // if this is a client with no tombstones then we subtract the number
- // of entries being affected by register-interest refresh
- if (imageState.isClient() && !getConcurrencyChecksEnabled()) {
- int destroyedEntriesCount = imageState.getDestroyedEntriesCount();
- if (result < destroyedEntriesCount) {
- logger.error("Incorrect region size: mapSize={},
destroyedEntriesCount={}.", result,
- destroyedEntriesCount);
+ synchronized (tombstoneCount) {
+ int result = getRegionMap().size();
+ // if this is a client with no tombstones then we subtract the number
+ // of entries being affected by register-interest refresh
+ if (imageState.isClient() && !getConcurrencyChecksEnabled()) {
+ int destroyedEntriesCount = imageState.getDestroyedEntriesCount();
+ if (result < destroyedEntriesCount) {
+ logger.error("Incorrect region size: mapSize={},
destroyedEntriesCount={}.", result,
+ destroyedEntriesCount);
+ }
+ return result - destroyedEntriesCount;
Review comment:
since we know returning a negative number can cause problems and is not
meaningful, I think we should return 0 after logging instead of returning a
negative number
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -2109,24 +2109,26 @@ int entryCount(Set<Integer> buckets, boolean estimate) {
@Override
public int getRegionSize() {
synchronized (getSizeGuard()) {
- int result = getRegionMap().size();
- // if this is a client with no tombstones then we subtract the number
- // of entries being affected by register-interest refresh
- if (imageState.isClient() && !getConcurrencyChecksEnabled()) {
- int destroyedEntriesCount = imageState.getDestroyedEntriesCount();
- if (result < destroyedEntriesCount) {
- logger.error("Incorrect region size: mapSize={},
destroyedEntriesCount={}.", result,
- destroyedEntriesCount);
+ synchronized (tombstoneCount) {
Review comment:
I think synchronizing on an atomic is an "odd" thing to do and is worth
you adding a comment here to explain the race it prevents. I think it would
even be worth having the comment refer to your new
regionSizeShouldAlwaysBePositive test
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
##########
@@ -2109,24 +2109,26 @@ int entryCount(Set<Integer> buckets, boolean estimate) {
@Override
public int getRegionSize() {
synchronized (getSizeGuard()) {
- int result = getRegionMap().size();
- // if this is a client with no tombstones then we subtract the number
- // of entries being affected by register-interest refresh
- if (imageState.isClient() && !getConcurrencyChecksEnabled()) {
- int destroyedEntriesCount = imageState.getDestroyedEntriesCount();
- if (result < destroyedEntriesCount) {
- logger.error("Incorrect region size: mapSize={},
destroyedEntriesCount={}.", result,
- destroyedEntriesCount);
+ synchronized (tombstoneCount) {
+ int result = getRegionMap().size();
+ // if this is a client with no tombstones then we subtract the number
+ // of entries being affected by register-interest refresh
+ if (imageState.isClient() && !getConcurrencyChecksEnabled()) {
+ int destroyedEntriesCount = imageState.getDestroyedEntriesCount();
+ if (result < destroyedEntriesCount) {
+ logger.error("Incorrect region size: mapSize={},
destroyedEntriesCount={}.", result,
+ destroyedEntriesCount);
+ }
+ return result - destroyedEntriesCount;
}
- return result - destroyedEntriesCount;
- }
- int tombstoneNumber = tombstoneCount.get();
- if (result < tombstoneNumber) {
- logger.error("Incorrect region size: mapSize={}, tombstoneCount={}.",
result,
- tombstoneNumber);
+ int tombstoneNumber = tombstoneCount.get();
+ if (result < tombstoneNumber) {
+ logger.error("Incorrect region size: mapSize={},
tombstoneCount={}.", result,
+ tombstoneNumber);
Review comment:
return 0 here also
--
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.
To unsubscribe, e-mail: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
