[
https://issues.apache.org/jira/browse/GEODE-7846?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17167034#comment-17167034
]
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_r461227033
##########
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:
If it's safe to assume that buckets aren't moving around during the
clear (which in this test seems to be the case) then it should be possible to
get the number of buckets on each member with
`region.getDataStore().getAllLocalBucketRegions()` and use that for
verification.
##########
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:
Might it be possible to instead move the call to this method (and
`incClearDuration()`, if it ends up staying where it currently is) inside the
`if (partitionedRegion.getDataStore() != null)` block in `clearRegionLocal()`,
just after the `doAfterClear()`, and guarded with a check on the size of
`clearedBuckets`? i.e. if no buckets are cleared, do not increment the stats?
That should avoid having to do quite so much checking in the method itself and
instead place the focus on calling it in the appropriate place, where it will
always increment the stats.
##########
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 think that the rest of the tests in the class are specifically testing
clear's interaction with writers (at least judging by test names) so it's
appropriate for these checks to be present in them, but in a test specifically
about stats, they risk causing confusion.
##########
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:
If all the checks were removed from the method, then it would turn into
just 'partitionedRegion.getCachePerfStats().incRegionClearCount()` which could
be called inline rather than having a separate method, especially considering
that it's only called once. Looking at how other stats are incremented, this
pattern of directly calling the method on the stats class inline when it's
appropriate to do so seems to be used almost everywhere, and using it here
would simplify the class a fair bit.
----------------------------------------------------------------
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, GeodeOperationAPI, 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)
