[geode] branch feature/GEODE-6630 updated: GEODE-6630: set allBucketsRecoveredFromDisk before starting logging thread.

[es...@apache.org]

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

eshu11 pushed a commit to branch feature/GEODE-6630
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/feature/GEODE-6630 by this push:
     new 86587b9  GEODE-6630: set allBucketsRecoveredFromDisk before starting logging thread.
86587b9 is described below

commit 86587b98ff94b187f7c3e9f9b6a094aa101221bb
Author: eshu <es...@pivotal.io>
AuthorDate: Tue Apr 16 16:36:37 2019 -0700

    GEODE-6630: set allBucketsRecoveredFromDisk before starting logging thread.
---
 .../internal/cache/PRHARedundancyProvider.java     | 32 +++++++++++++++------
 .../internal/cache/PRHARedundancyProviderTest.java | 33 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 9 deletions(-)

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 2942154..f519914 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
@@ -1685,6 +1685,10 @@ public class PRHARedundancyProvider {
     final ProxyBucketRegion[] proxyBucketArray =
         persistentLeader.getRegionAdvisor().getProxyBucketArray();
 
+    if (proxyBucketArray.length == 0) {
+      throw new IllegalStateException("Unexpected empty proxy bucket array");
+    }
+
     for (ProxyBucketRegion proxyBucket : proxyBucketArray) {
       proxyBucket.initializePersistenceAdvisor();
     }
@@ -1711,9 +1715,8 @@ public class PRHARedundancyProvider {
     /*
      * Start the redundancy logger before recovering any proxy buckets.
      */
-    startRedundancyLogger(proxyBucketArray.length);
+    createRedundancyLoggerAndStartLoggingThread(proxyBucketArray.length);
 
-    allBucketsRecoveredFromDisk = new CountDownLatch(proxyBucketArray.length);
     /*
      * Spawn a separate thread for bucket that we previously hosted to recover that bucket.
      *
@@ -1784,13 +1787,24 @@ public class PRHARedundancyProvider {
     // }
   }
 
-  void startRedundancyLogger(int proxyBuckets) {
-    if (proxyBuckets > 0) {
-      redundancyLogger = new RedundancyLogger(this);
-      Thread loggingThread = new LoggingThread(
-          "RedundancyLogger for region " + this.prRegion.getName(), false, this.redundancyLogger);
-      loggingThread.start();
-    }
+  void createRedundancyLoggerAndStartLoggingThread(int proxyBuckets) {
+    redundancyLogger = createRedundancyLogger();
+    allBucketsRecoveredFromDisk = createAllBucketsRecoveredFromDisk(proxyBuckets);
+    Thread loggingThread = createRedundancyLoggingThread();
+    loggingThread.start();
+  }
+
+  LoggingThread createRedundancyLoggingThread() {
+    return new LoggingThread(
+        "RedundancyLogger for region " + prRegion.getName(), false, redundancyLogger);
+  }
+
+  CountDownLatch createAllBucketsRecoveredFromDisk(int proxyBuckets) {
+    return new CountDownLatch(proxyBuckets);
+  }
+
+  RedundancyLogger createRedundancyLogger() {
+    return new RedundancyLogger(this);
   }
 
   /**
diff --git a/geode-core/src/test/java/org/apache/geode/internal/cache/PRHARedundancyProviderTest.java b/geode-core/src/test/java/org/apache/geode/internal/cache/PRHARedundancyProviderTest.java
index e650cbc..5c393ff 100644
--- a/geode-core/src/test/java/org/apache/geode/internal/cache/PRHARedundancyProviderTest.java
+++ b/geode-core/src/test/java/org/apache/geode/internal/cache/PRHARedundancyProviderTest.java
@@ -14,7 +14,12 @@
  */
 package org.apache.geode.internal.cache;
 
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.catchThrowable;
 import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
@@ -23,6 +28,10 @@ import java.util.concurrent.CountDownLatch;
 
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.InOrder;
+
+import org.apache.geode.internal.cache.partitioned.RedundancyLogger;
+import org.apache.geode.internal.logging.LoggingThread;
 
 
 public class PRHARedundancyProviderTest {
@@ -48,4 +57,28 @@ public class PRHARedundancyProviderTest {
 
     verify(provider.allBucketsRecoveredFromDisk).await();
   }
+
+  @Test
+  public void allBucketsRecoveredFromDiskCountDownLatchIsNotSetIfFailedToCreateRedundancyLogger() {
+    doThrow(new RuntimeException()).when(provider).createRedundancyLogger();
+
+    Throwable thrown =
+        catchThrowable(() -> provider.createRedundancyLoggerAndStartLoggingThread(1));
+
+    assertThat(provider.allBucketsRecoveredFromDisk).isNull();
+    assertThat(thrown).isInstanceOf(RuntimeException.class);
+  }
+
+  @Test
+  public void allBucketsRecoveredFromDiskIsSetBeforeLoggingThreadStarts() {
+    LoggingThread thread = mock(LoggingThread.class);
+    doReturn(mock(RedundancyLogger.class)).when(provider).createRedundancyLogger();
+    doReturn(thread).when(provider).createRedundancyLoggingThread();
+
+    provider.createRedundancyLoggerAndStartLoggingThread(1);
+
+    InOrder inOrder = inOrder(provider, thread);
+    inOrder.verify(provider).createAllBucketsRecoveredFromDisk(1);
+    inOrder.verify(thread).start();
+  }
 }