[
https://issues.apache.org/jira/browse/GEODE-7846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17163822#comment-17163822
]
ASF GitHub Bot commented on GEODE-7846:
---------------------------------------
DonalEvans commented on a change in pull request #5391:
URL: https://github.com/apache/geode/pull/5391#discussion_r459601065
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -162,6 +162,7 @@ protected void
waitForPrimary(PartitionedRegion.RetryTimeKeeper retryTimer) {
doAfterClear(regionEvent);
}
}
+ incClearCount();
Review comment:
I feel like the calls to `incClearCount()` and `incClearDuration()`
should be made at the same time, since there should be a consistent approach to
when a clear is "done."
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -367,12 +370,38 @@ void doClear(RegionEventImpl regionEvent, boolean
cacheWrite) {
releaseLockForClear(regionEvent);
}
}
-
} finally {
releaseDistributedClearLock(lockName);
+ incClearDuration(System.nanoTime() - clearStartTime);
+ }
+ }
+
+ void incClearCount() {
+ if (partitionedRegion != null && partitionedRegion.getDataStore() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions().size()
!= 0) {
+ CachePerfStats stats = partitionedRegion.getCachePerfStats();
+ if (stats != null) {
+ logger.info("BR inc PR Region count:" + stats.getClass().getName() +
":"
+ + partitionedRegion.getFullPath(), new Exception());
+ stats.incClearCount();
+ }
}
}
+ void incClearDuration(long durationNanos) {
+ if (partitionedRegion != null &&
partitionedRegion.getTotalNumberOfBuckets() != 0) {
Review comment:
See my other comment about the possibility of hitting the scenario being
checked for. Is it actually possible that we get to where this method is called
and `getTotalNumberOfBuckets()` is zero? If it is, do we want to throw an
exception instead of just quietly not incrementing the stats?
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -367,12 +370,38 @@ void doClear(RegionEventImpl regionEvent, boolean
cacheWrite) {
releaseLockForClear(regionEvent);
}
}
-
} finally {
releaseDistributedClearLock(lockName);
+ incClearDuration(System.nanoTime() - clearStartTime);
+ }
+ }
+
+ void incClearCount() {
+ if (partitionedRegion != null && partitionedRegion.getDataStore() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions().size()
!= 0) {
+ CachePerfStats stats = partitionedRegion.getCachePerfStats();
+ if (stats != null) {
+ logger.info("BR inc PR Region count:" + stats.getClass().getName() +
":"
+ + partitionedRegion.getFullPath(), new Exception());
+ stats.incClearCount();
+ }
}
}
+ void incClearDuration(long durationNanos) {
+ if (partitionedRegion != null &&
partitionedRegion.getTotalNumberOfBuckets() != 0) {
+ CachePerfStats stats = partitionedRegion.getCachePerfStats();
+ if (stats != null) {
+ logger.info("BR inc PR Duration by + " + durationNanos + " ns:" +
stats.getClass().getName()
Review comment:
Remove this logging.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -367,12 +370,38 @@ void doClear(RegionEventImpl regionEvent, boolean
cacheWrite) {
releaseLockForClear(regionEvent);
}
}
-
} finally {
releaseDistributedClearLock(lockName);
+ incClearDuration(System.nanoTime() - clearStartTime);
+ }
+ }
+
+ void incClearCount() {
+ if (partitionedRegion != null && partitionedRegion.getDataStore() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions().size()
!= 0) {
Review comment:
I'm not sure that all these null checks are required. I would think that
if the region is null at this point, then we would expect that something has
gone wrong and that we should throw an exception rather than just silently not
incrementing the stats. Also, I'm not sure I understand why we only increment
the stats if `getAllLocalBucketRegions()` returns a non-empty set. Are there
scenarios where we could call clear, it could complete without an exception,
bringing us to this method, and then `getAllLocalBucketRegions()` returns empty?
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java
##########
@@ -330,6 +337,101 @@ public void
normalClearFromAccessorWithoutWriterButWithWriterOnDataStore() {
.isEqualTo(0);
}
+ @Test
+ public void normalClearFromDataStoreUpdatesStats() {
+ configureServers(false, true);
+ client1.invoke(this::initClientCache);
+ client2.invoke(this::initClientCache);
+
+ // Verify no clears have been recorded in stats
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ long clearCount = bucket.getCachePerfStats().getRegionClearCount();
+ assertThat(clearCount).isEqualTo(0);
+ }
+ });
+
+ accessor.invoke(() -> feed(false));
+ verifyServerRegionSize(NUM_ENTRIES);
+ dataStore1.invoke(() -> getRegion(false).clear());
+ verifyServerRegionSize(0);
+
+ // Verify the stats were properly updated for the bucket regions
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
Review comment:
Rather than just comparing the clear counts of the buckets between
eachother, it might be better to compare all of them to the expected clear
count. The current check just asserts that the clear counts for all the buckets
are equal, but not that that value is necessarily correct.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java
##########
@@ -330,6 +337,101 @@ public void
normalClearFromAccessorWithoutWriterButWithWriterOnDataStore() {
.isEqualTo(0);
}
+ @Test
+ public void normalClearFromDataStoreUpdatesStats() {
+ configureServers(false, true);
+ client1.invoke(this::initClientCache);
+ client2.invoke(this::initClientCache);
+
+ // Verify no clears have been recorded in stats
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ long clearCount = bucket.getCachePerfStats().getRegionClearCount();
+ assertThat(clearCount).isEqualTo(0);
+ }
+ });
+
+ accessor.invoke(() -> feed(false));
+ verifyServerRegionSize(NUM_ENTRIES);
+ dataStore1.invoke(() -> getRegion(false).clear());
+ verifyServerRegionSize(0);
+
+ // Verify the stats were properly updated for the bucket regions
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ if (clearCount == 0) {
+ clearCount = bucket.getCachePerfStats().getBucketClearCount();
+ }
+
assertThat(bucket.getCachePerfStats().getBucketClearCount()).isEqualTo(clearCount)
+ .isNotEqualTo(0);
+ }
+
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(1);
+
assertThat(region.getCachePerfStats().getPartitionedRegionClearDuration()).isGreaterThan(0);
+ });
+
+ dataStore2.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ if (clearCount == 0) {
+ clearCount = bucket.getCachePerfStats().getBucketClearCount();
+ }
+
assertThat(bucket.getCachePerfStats().getBucketClearCount()).isEqualTo(clearCount)
+ .isNotEqualTo(0);
+ }
+
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(1);
Review comment:
This invocation is executed on all three datastores. It might simplify
the test to extract it to a method.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/RegionPerfStats.java
##########
@@ -519,9 +519,15 @@ public void incEvictWorkTime(long delta) {
cachePerfStats.incEvictWorkTime(delta);
}
+ @Override
+ public void incRegionClearCount() {
+ stats.incLong(regionClearsId, 1L);
+ cachePerfStats.incClearCount();
+ }
+
@Override
public void incClearCount() {
- stats.incLong(clearsId, 1L);
+ stats.incLong(bucketClearsId, 1L);
Review comment:
I think this may possibly be incorrect. `incRegionClearCount()` is being
overridden from `RegionStats` but every other method in this class is
overriding methods from `CachePerfStats`. I think the correct methods might be
`incClearCount()` which calls `cachePerfStats.incClearCount()` and
`incBucketClearCount()` which calls `cachePerfStats.incBucketClearCount()`.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -367,12 +370,38 @@ void doClear(RegionEventImpl regionEvent, boolean
cacheWrite) {
releaseLockForClear(regionEvent);
}
}
-
} finally {
releaseDistributedClearLock(lockName);
+ incClearDuration(System.nanoTime() - clearStartTime);
+ }
+ }
+
+ void incClearCount() {
+ if (partitionedRegion != null && partitionedRegion.getDataStore() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions() != null
+ && partitionedRegion.getDataStore().getAllLocalBucketRegions().size()
!= 0) {
+ CachePerfStats stats = partitionedRegion.getCachePerfStats();
+ if (stats != null) {
+ logger.info("BR inc PR Region count:" + stats.getClass().getName() +
":"
Review comment:
Remove this logging.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java
##########
@@ -170,6 +170,13 @@ private void verifyClientRegionSize(int expectedNum) {
return destroys;
};
+ SerializableCallableIF<Integer> getStatClearCount = () -> {
Review comment:
This method is never used.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/CachePerfStats.java
##########
@@ -442,7 +451,10 @@
f.createIntCounter("retries",
"Number of times a concurrent destroy followed by a create has
caused an entry operation to need to retry.",
"operations"),
- f.createLongCounter("clears", clearsDesc, "operations"),
+ f.createLongCounter("regionClears", regionClearsDesc,
"operations"),
Review comment:
If the name of this statistic is changed, then
`geode-docs/reference/statistics_list.html.md.erb` should be updated to reflect
the new name. Also, it might be good to document the newly added statistics too.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/RegionStats.java
##########
@@ -135,7 +135,9 @@
void incEvictWorkTime(long delta);
- void incClearCount();
+ void incBucketClearCount();
+
+ void incRegionClearCount();
Review comment:
This method should have remained as `incClearCount()`, I think, to
remain consistent with `CachePerfStats`.
##########
File path:
geode-core/src/distributedTest/java/org/apache/geode/internal/cache/PartitionedRegionClearDUnitTest.java
##########
@@ -330,6 +337,101 @@ public void
normalClearFromAccessorWithoutWriterButWithWriterOnDataStore() {
.isEqualTo(0);
}
+ @Test
+ public void normalClearFromDataStoreUpdatesStats() {
+ configureServers(false, true);
+ client1.invoke(this::initClientCache);
+ client2.invoke(this::initClientCache);
+
+ // Verify no clears have been recorded in stats
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ long clearCount = bucket.getCachePerfStats().getRegionClearCount();
+ assertThat(clearCount).isEqualTo(0);
+ }
+ });
+
+ accessor.invoke(() -> feed(false));
+ verifyServerRegionSize(NUM_ENTRIES);
+ dataStore1.invoke(() -> getRegion(false).clear());
+ verifyServerRegionSize(0);
+
+ // Verify the stats were properly updated for the bucket regions
+ dataStore1.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ if (clearCount == 0) {
+ clearCount = bucket.getCachePerfStats().getBucketClearCount();
+ }
+
assertThat(bucket.getCachePerfStats().getBucketClearCount()).isEqualTo(clearCount)
+ .isNotEqualTo(0);
+ }
+
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(1);
+
assertThat(region.getCachePerfStats().getPartitionedRegionClearDuration()).isGreaterThan(0);
+ });
+
+ dataStore2.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ if (clearCount == 0) {
+ clearCount = bucket.getCachePerfStats().getBucketClearCount();
+ }
+
assertThat(bucket.getCachePerfStats().getBucketClearCount()).isEqualTo(clearCount)
+ .isNotEqualTo(0);
+ }
+
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(1);
+ });
+
+ dataStore3.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+ long clearCount = 0L;
+
+ for (BucketRegion bucket :
region.getDataStore().getAllLocalBucketRegions()) {
+ if (clearCount == 0) {
+ clearCount = bucket.getCachePerfStats().getBucketClearCount();
+ }
+
assertThat(bucket.getCachePerfStats().getBucketClearCount()).isEqualTo(clearCount)
+ .isNotEqualTo(0);
+ }
+
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(1);
+ });
+
+
+ // The accessor shouldn't increment the region clear count
+ accessor.invoke(() -> {
+ PartitionedRegion region = (PartitionedRegion) getRegion(false);
+
assertThat(region.getCachePerfStats().getRegionClearCount()).isEqualTo(0);
+ });
+
+ // do the region destroy to compare that the same callbacks will be
triggered
+ dataStore1.invoke(() -> {
+ Region region = getRegion(false);
+ region.destroyRegion();
+ });
+
+
assertThat(dataStore1.invoke(getWriterDestroys)).isEqualTo(dataStore1.invoke(getWriterClears))
+ .isEqualTo(0);
+
assertThat(dataStore2.invoke(getWriterDestroys)).isEqualTo(dataStore2.invoke(getWriterClears))
+ .isEqualTo(0);
+
assertThat(dataStore3.invoke(getWriterDestroys)).isEqualTo(dataStore3.invoke(getWriterClears))
+ .isEqualTo(0);
+
assertThat(accessor.invoke(getWriterDestroys)).isEqualTo(accessor.invoke(getWriterClears))
+ .isEqualTo(1);
+
+ assertThat(accessor.invoke(getBucketRegionWriterDestroys))
+ .isEqualTo(accessor.invoke(getBucketRegionWriterClears))
+ .isEqualTo(0);
Review comment:
I'm not sure what this section has to do with stats. I think it can
safely be removed from this test case.
----------------------------------------------------------------
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:
us...@infra.apache.org
> Clear in Partitioned Region should have its own stats
> -----------------------------------------------------
>
> Key: GEODE-7846
> URL: https://issues.apache.org/jira/browse/GEODE-7846
> Project: Geode
> Issue Type: Improvement
> Components: core
> Reporter: Xiaojian Zhou
> Priority: Major
> Labels: GeodeCommons, pull-request-available
>
> Clear operation in PR should have its own stats:
> 1) clear operation executed.
> 2) clear operation failed
> 3) clear messages sends to buckets
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
