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 5db56c8  fix review comments.
5db56c8 is described below

commit 5db56c80918c6a8132a28c8efa8af1b35a025632
Author: eshu <[email protected]>
AuthorDate: Thu Apr 18 09:18:57 2019 -0700

    fix review comments.
---
 .../internal/cache/PRHARedundancyProvider.java     | 40 ++-------------
 .../partitioned/PersistentBucketRecoverer.java     | 58 ++++++++++++++++++----
 .../partitioned/PersistentBucketRecovererTest.java |  3 +-
 3 files changed, 53 insertions(+), 48 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 add48cf..7ec3523 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
@@ -1708,15 +1708,11 @@ public class PRHARedundancyProvider {
     ArrayList<ProxyBucketRegion> bucketsHostedLocally =
         new ArrayList<ProxyBucketRegion>(proxyBucketArray.length);
 
-
+    persistentBucketRecoverer = 
createPersistentBucketRecoverer(proxyBucketArray.length);
     /*
      * Start the redundancy logger before recovering any proxy buckets.
      */
-    persistentBucketRecoverer = 
createPersistentBucketRecoverer(proxyBucketArray.length);
-    Thread loggingThread = new LoggingThread(
-        "PersistentBucketRecoverer for region " + prRegion.getName(), false,
-        persistentBucketRecoverer);
-    loggingThread.start();
+    persistentBucketRecoverer.startLoggingThread();
 
     /*
      * Spawn a separate thread for bucket that we previously hosted to recover 
that bucket.
@@ -1738,7 +1734,6 @@ public class PRHARedundancyProvider {
       if (proxyBucket.getPersistenceAdvisor().wasHosting()) {
         final RecoveryRunnable recoveryRunnable = new RecoveryRunnable(this) {
 
-
           @Override
           public void run() {
             // Fix for 44551 - make sure that we always count down
@@ -1989,22 +1984,8 @@ public class PRHARedundancyProvider {
    */
   protected void waitForPersistentBucketRecoveryOrClose() {
     if (getPersistentBucketRecoverer() != null) {
-      boolean interrupted = false;
-      while (true) {
-        try {
-          this.prRegion.getCancelCriterion().checkCancelInProgress(null);
-          boolean done = getPersistentBucketRecoverer().await(
-              PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION, 
TimeUnit.MILLISECONDS);
-          if (done) {
-            break;
-          }
-        } catch (InterruptedException e) {
-          interrupted = true;
-        }
-      }
-      if (interrupted) {
-        Thread.currentThread().interrupt();
-      }
+      getPersistentBucketRecoverer().await(
+          PartitionedRegionHelper.DEFAULT_WAIT_PER_RETRY_ITERATION, 
TimeUnit.MILLISECONDS);
     }
 
     List<PartitionedRegion> colocatedRegions =
@@ -2020,18 +2001,7 @@ public class PRHARedundancyProvider {
    */
   protected void waitForPersistentBucketRecovery() {
     if (getPersistentBucketRecoverer() != null) {
-      boolean interrupted = false;
-      while (true) {
-        try {
-          getPersistentBucketRecoverer().await();
-          break;
-        } catch (InterruptedException e) {
-          interrupted = true;
-        }
-      }
-      if (interrupted) {
-        Thread.currentThread().interrupt();
-      }
+      getPersistentBucketRecoverer().await();
     }
   }
 
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
 
b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
index 813517d..021c03a 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
@@ -39,15 +39,15 @@ import org.apache.geode.internal.cache.ProxyBucketRegion;
 import org.apache.geode.internal.cache.persistence.PersistentMemberID;
 import org.apache.geode.internal.cache.persistence.PersistentStateListener;
 import org.apache.geode.internal.logging.LogService;
+import org.apache.geode.internal.logging.LoggingThread;
 import org.apache.geode.internal.net.SocketCreator;
 import org.apache.geode.internal.process.StartupStatus;
 import org.apache.geode.internal.util.TransformUtils;
 
 /**
  * Consolidates logging during the recovery of ProxyRegionBuckets that are not 
hosted by this
- * member. This logger is meant to run in its own thread and utilizes the 
PRHARedundancyProvider's
- * count down latch in order to determine when it is finished.
- *
+ * member. The logger is meant to run in its own thread.
+ * It uses a count down latch to determine whether the recovery is finished.
  */
 public class PersistentBucketRecoverer extends RecoveryRunnable implements 
PersistentStateListener {
 
@@ -95,12 +95,20 @@ public class PersistentBucketRecoverer extends 
RecoveryRunnable implements Persi
     allBucketsRecoveredFromDisk = new CountDownLatch(proxyBuckets);
     membershipChanged = true;
     addListeners();
+
   }
 
   List<PartitionedRegion> getColocatedChildRegions(PartitionedRegion 
baseRegion) {
     return ColocationHelper.getColocatedChildRegions(baseRegion);
   }
 
+  public void startLoggingThread() {
+    Thread loggingThread = new LoggingThread(
+        "PersistentBucketRecoverer for region " + 
redundancyProvider.prRegion.getName(), false,
+        this);
+    loggingThread.start();
+  }
+
   /**
    * Called when a member comes online for a bucket.
    */
@@ -157,7 +165,7 @@ public class PersistentBucketRecoverer extends 
RecoveryRunnable implements Persi
   public void run2() {
     try {
       boolean warningLogged = false;
-      while (this.allBucketsRecoveredFromDisk.getCount() > 0) {
+      while (getLatchCount() > 0) {
         int sleepMillis = SLEEP_PERIOD;
         // reduce the first log time from 15secs so that higher layers can
         // report sooner to user
@@ -342,8 +350,7 @@ public class PersistentBucketRecoverer extends 
RecoveryRunnable implements Persi
       Map<PersistentMemberID, Set<Integer>> offlineMembers = 
getMembersToWaitFor(true);
       Map<PersistentMemberID, Set<Integer>> allMembersToWaitFor = 
getMembersToWaitFor(false);
 
-      boolean thereAreBucketsToBeRecovered =
-          
(PersistentBucketRecoverer.this.allBucketsRecoveredFromDisk.getCount() > 0);
+      boolean thereAreBucketsToBeRecovered = (getLatchCount() > 0);
 
       /*
        * Log any offline members the region is waiting for.
@@ -408,12 +415,37 @@ public class PersistentBucketRecoverer extends 
RecoveryRunnable implements Persi
     }
   }
 
-  public boolean await(long timeout, TimeUnit unit) throws 
InterruptedException {
-    return allBucketsRecoveredFromDisk.await(timeout, unit);
+  public void await(long timeout, TimeUnit unit) {
+    boolean interrupted = false;
+    while (true) {
+      try {
+        
redundancyProvider.prRegion.getCancelCriterion().checkCancelInProgress(null);
+        boolean done = allBucketsRecoveredFromDisk.await(timeout, unit);
+        if (done) {
+          break;
+        }
+      } catch (InterruptedException e) {
+        interrupted = true;
+      }
+    }
+    if (interrupted) {
+      Thread.currentThread().interrupt();
+    }
   }
 
-  public void await() throws InterruptedException {
-    allBucketsRecoveredFromDisk.await();
+  public void await() {
+    boolean interrupted = false;
+    while (true) {
+      try {
+        getAllBucketsRecoveredFromDiskLatch().await();
+        break;
+      } catch (InterruptedException e) {
+        interrupted = true;
+      }
+    }
+    if (interrupted) {
+      Thread.currentThread().interrupt();
+    }
   }
 
   public void countDown() {
@@ -421,12 +453,16 @@ public class PersistentBucketRecoverer extends 
RecoveryRunnable implements Persi
   }
 
   public boolean hasRecoveryCompleted() {
-    if (allBucketsRecoveredFromDisk.getCount() > 0) {
+    if (getLatchCount() > 0) {
       return false;
     }
     return true;
   }
 
+  long getLatchCount() {
+    return allBucketsRecoveredFromDisk.getCount();
+  }
+
   CountDownLatch getAllBucketsRecoveredFromDiskLatch() {
     return allBucketsRecoveredFromDisk;
   }
diff --git 
a/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecovererTest.java
 
b/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecovererTest.java
index c2afc3a..0500d47 100644
--- 
a/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecovererTest.java
+++ 
b/geode-core/src/test/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecovererTest.java
@@ -42,7 +42,6 @@ public class PersistentBucketRecovererTest {
         new PersistentBucketRecoverer(provider, numberOfProxyBuckets);
 
     assertThat(recoverer.getAllBucketsRecoveredFromDiskLatch()).isNotNull();
-    assertThat(recoverer.getAllBucketsRecoveredFromDiskLatch().getCount())
-        .isEqualTo(numberOfProxyBuckets);
+    assertThat(recoverer.getLatchCount()).isEqualTo(numberOfProxyBuckets);
   }
 }

Reply via email to