[geode] branch develop updated: GEODE-7963: solution for faulty bucket metrics (#5000)

[mi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

mivanac pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new 1b4b60c  GEODE-7963: solution for faulty bucket metrics (#5000)
1b4b60c is described below

commit 1b4b60ca66867a995a593eb0727404e0d89ab9c9
Author: Mario Ivanac <48...@users.noreply.github.com>
AuthorDate: Sun May 10 10:45:42 2020 +0200

    GEODE-7963: solution for faulty bucket metrics (#5000)
    
    * GEODE-7963: solution for faulty bucket metrics
    
    * GEODE-7963: added test to reproduce fault
    
    * GEODE-7963: added UT
    
    * GEODE-7963: update after comments
    
    * GEODE-7963: small updates
---
 .../management/MemberMXBeanDistributedTest.java    | 129 +++++++++++++++++++++
 .../geode/internal/cache/GemFireCacheImpl.java     |   3 +-
 .../geode/internal/cache/InternalRegion.java       |   4 +
 .../apache/geode/internal/cache/LocalRegion.java   |  10 ++
 .../internal/cache/PRHARedundancyProvider.java     |   5 +
 .../geode/internal/cache/PartitionedRegion.java    |  22 +++-
 .../internal/cache/PartitionedRegionTest.java      |  24 ++++
 7 files changed, 195 insertions(+), 2 deletions(-)

diff --git a/geode-core/src/distributedTest/java/org/apache/geode/management/MemberMXBeanDistributedTest.java b/geode-core/src/distributedTest/java/org/apache/geode/management/MemberMXBeanDistributedTest.java
new file mode 100644
index 0000000..55a82a2
--- /dev/null
+++ b/geode-core/src/distributedTest/java/org/apache/geode/management/MemberMXBeanDistributedTest.java
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management;
+
+import static org.apache.geode.test.awaitility.GeodeAwaitility.await;
+import static org.junit.Assert.assertEquals;
+
+import java.io.Serializable;
+
+import javax.management.ObjectName;
+
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestName;
+
+import org.apache.geode.cache.Cache;
+import org.apache.geode.cache.partition.PartitionRegionHelper;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+import org.apache.geode.test.junit.categories.GfshTest;
+import org.apache.geode.test.junit.rules.GfshCommandRule;
+import org.apache.geode.test.junit.rules.serializable.SerializableTestName;
+
+@Category({GfshTest.class})
+public class MemberMXBeanDistributedTest implements
+    Serializable {
+
+  private static MemberVM locator;
+  private static MemberVM server1;
+  private static MemberVM server2;
+  private static MemberVM server3;
+  private static MemberVM server4;
+
+  @ClassRule
+  public static ClusterStartupRule lsRule = new ClusterStartupRule();
+
+  @ClassRule
+  public static GfshCommandRule gfsh = new GfshCommandRule();
+
+  @Rule
+  public TestName testName = new SerializableTestName();
+
+  @BeforeClass
+  public static void before() throws Exception {
+    locator = lsRule.startLocatorVM(0);
+    server1 = lsRule.startServerVM(1, "", locator.getPort());
+    server2 = lsRule.startServerVM(2, "", locator.getPort());
+    server3 = lsRule.startServerVM(3, "", locator.getPort());
+    server4 = lsRule.startServerVM(4, "", locator.getPort());
+
+    gfsh.connectAndVerify(locator);
+  }
+
+  @Test
+  public void testBucketCount() {
+    String regionName = "testCreateRegion";
+
+    gfsh.executeAndAssertThat("create region"
+        + " --name=" + regionName
+        + " --type=PARTITION_PERSISTENT"
+        + " --total-num-buckets=1000").statusIsSuccess();
+
+    server1.invoke(() -> createBuckets(regionName));
+    server2.invoke(() -> createBuckets(regionName));
+    server3.invoke(() -> createBuckets(regionName));
+    server4.invoke(() -> createBuckets(regionName));
+
+    await().untilAsserted(() -> {
+      final int sumOfBuckets = server1.invoke(() -> getBucketsInitialized()) +
+          server2.invoke(() -> getBucketsInitialized()) +
+          server3.invoke(() -> getBucketsInitialized()) +
+          server4.invoke(() -> getBucketsInitialized());
+      assertEquals("Expected bucket count is 1000, and actual count is " + sumOfBuckets,
+          sumOfBuckets, 1000);
+    });
+
+    for (int i = 1; i < 4; i++) {
+      gfsh.executeAndAssertThat("create region"
+          + " --name=" + regionName + i
+          + " --type=PARTITION_PERSISTENT"
+          + " --total-num-buckets=1000"
+          + " --colocated-with=" + regionName).statusIsSuccess();
+    }
+
+    await().untilAsserted(() -> {
+      final int sumOfBuckets = server1.invoke(() -> getBucketsInitialized()) +
+          server2.invoke(() -> getBucketsInitialized()) +
+          server3.invoke(() -> getBucketsInitialized()) +
+          server4.invoke(() -> getBucketsInitialized());
+      assertEquals("Expected bucket count is 4000, and actual count is " + sumOfBuckets,
+          sumOfBuckets, 4000);
+    });
+
+  }
+
+  private int getBucketsInitialized() {
+    Cache cache = ClusterStartupRule.getCache();
+
+    DistributedMember member = cache.getDistributedSystem().getDistributedMember();
+    ManagementService mgmtService = ManagementService.getManagementService(cache);
+    ObjectName memberMBeanName = mgmtService.getMemberMBeanName(member);
+    MemberMXBean memberMXBean = mgmtService.getMBeanInstance(memberMBeanName, MemberMXBean.class);
+
+    return memberMXBean.getTotalBucketCount();
+  }
+
+  private void createBuckets(String regionName) {
+    Cache cache = ClusterStartupRule.getCache();
+    PartitionRegionHelper.assignBucketsToPartitions(cache.getRegion(regionName));
+  }
+
+}
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
index caa2532..22a3572 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
@@ -3058,8 +3058,9 @@ public class GemFireCacheImpl implements InternalCache, InternalClientCache, Has
     invokeRegionAfter(region);
 
     // Putting the callback here to avoid creating RegionMBean in case of Exception
-    if (!region.isInternalRegion()) {
+    if (!region.isRegionCreateNotified() && !region.isInternalRegion()) {
       system.handleResourceEvent(ResourceEvent.REGION_CREATE, region);
+      region.setRegionCreateNotified(true);
     }
 
     return cast(region);
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/InternalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/InternalRegion.java
index 097aace..4ae752b 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/InternalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/InternalRegion.java
@@ -459,4 +459,8 @@ public interface InternalRegion extends Region, HasCachePerfStats, RegionEntryCo
    * @return true if synchronization should be attempted
    */
   boolean shouldSyncForCrashedMember(InternalDistributedMember id);
+
+  boolean isRegionCreateNotified();
+
+  void setRegionCreateNotified(boolean notified);
 }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index 48aeefe..11b5c9b 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -11047,6 +11047,16 @@ public class LocalRegion extends AbstractRegion implements LoaderHelperFactory,
     KEYS, VALUES, ENTRIES
   }
 
+  @Override
+  public boolean isRegionCreateNotified() {
+    return false;
+  }
+
+  @Override
+  public void setRegionCreateNotified(boolean notified) {
+    // do nothing
+  }
+
   /**
    * Used by {@link #foreachRegionEntry}.
    *
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java b/geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java
index f18cb49..e07ab55 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/PRHARedundancyProvider.java
@@ -1657,6 +1657,11 @@ public class PRHARedundancyProvider {
       for (ProxyBucketRegion proxyBucket : bucketsHostedLocally) {
         proxyBucket.waitForPrimaryPersistentRecovery();
       }
+
+      if (!partitionedRegion.isInternalRegion() && !bucketsNotHostedLocally.isEmpty()) {
+        partitionedRegion.notifyRegionCreated();
+      }
+
       for (ProxyBucketRegion proxyBucket : bucketsNotHostedLocally) {
         proxyBucket.recoverFromDiskRecursively();
       }
diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
index 9d7ad5e..d4bbf10 100755
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
@@ -154,6 +154,7 @@ import org.apache.geode.distributed.internal.OperationExecutors;
 import org.apache.geode.distributed.internal.ProfileListener;
 import org.apache.geode.distributed.internal.ReplyException;
 import org.apache.geode.distributed.internal.ReplyProcessor21;
+import org.apache.geode.distributed.internal.ResourceEvent;
 import org.apache.geode.distributed.internal.locks.DLockRemoteToken;
 import org.apache.geode.distributed.internal.locks.DLockService;
 import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
@@ -744,6 +745,8 @@ public class PartitionedRegion extends LocalRegion
 
   private final PartitionedRegionRedundancyTracker redundancyTracker;
 
+  private boolean regionCreationNotified;
+
   /**
    * Constructor for a PartitionedRegion. This has an accessor (Region API) functionality and
    * contains a datastore for actual storage. An accessor can act as a local cache by having a local
@@ -856,7 +859,7 @@ public class PartitionedRegion extends LocalRegion
       this.isShadowPR = true;
       this.parallelGatewaySender = internalRegionArgs.getParallelGatewaySender();
     }
-
+    this.regionCreationNotified = false;
 
     /*
      * Start persistent profile logging if we are a persistent region.
@@ -10188,4 +10191,21 @@ public class PartitionedRegion extends LocalRegion
   public SenderIdMonitor getSenderIdMonitor() {
     return senderIdMonitor;
   }
+
+  @Override
+  public boolean isRegionCreateNotified() {
+    return this.regionCreationNotified;
+  }
+
+  @Override
+  public void setRegionCreateNotified(boolean notified) {
+    this.regionCreationNotified = notified;
+  };
+
+  void notifyRegionCreated() {
+    if (regionCreationNotified)
+      return;
+    this.getSystem().handleResourceEvent(ResourceEvent.REGION_CREATE, this);
+    this.regionCreationNotified = true;
+  }
 }
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
index ae580e3..742db8a 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
@@ -622,6 +622,30 @@ public class PartitionedRegionTest {
     assertThat(failures.contains(1)).isFalse();
   }
 
+  @Test
+  public void testGetRegionCreateNotification() {
+    partitionedRegion = new PartitionedRegion("region", attributesFactory.create(), null, cache,
+        mock(InternalRegionArguments.class), disabledClock(), ColocationLoggerFactory.create());
+
+    assertThat(partitionedRegion.isRegionCreateNotified()).isFalse();
+
+    partitionedRegion.setRegionCreateNotified(true);
+
+    assertThat(partitionedRegion.isRegionCreateNotified()).isTrue();
+  }
+
+  @Test
+  public void testNotifyRegionCreated() {
+    partitionedRegion = new PartitionedRegion("region", attributesFactory.create(), null, cache,
+        mock(InternalRegionArguments.class), disabledClock(), ColocationLoggerFactory.create());
+
+    assertThat(partitionedRegion.isRegionCreateNotified()).isFalse();
+
+    partitionedRegion.notifyRegionCreated();
+
+    assertThat(partitionedRegion.isRegionCreateNotified()).isTrue();
+  }
+
   private static <K> Set<K> asSet(K... values) {
     Set<K> set = new HashSet<>();
     Collections.addAll(set, values);