BOOKKEEPER-911: Fix TestReplicationWorker tests in master

Author: Matteo Merli <[email protected]> Reviewers: Sijie Guo <[email protected]>, Flavio Junqueira <[email protected]> Closes apache#29 from merlimat/bk-911
rdhabalia · Apr 27, 2016 · 91595fc · 91595fc
1 parent 10cab08
commit 91595fc
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 10 deletions.
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java
@@ -73,6 +73,7 @@ public class ReplicationWorker implements Runnable {
     private final ServerConfiguration conf;
     private final ZooKeeper zkc;
     private volatile boolean workerRunning = false;
+    private volatile boolean isInReadOnlyMode = false;
     final private BookKeeperAdmin admin;
     private final LedgerChecker ledgerChecker;
     private final BookieSocketAddress targetBookie;
@@ -186,9 +187,12 @@ private static void waitBackOffTime() {
 
     private void waitTillTargetBookieIsWritable() {
         LOG.info("Waiting for target bookie {} to be back in read/write mode", targetBookie);
-        while (admin.getReadOnlyBookies().contains(targetBookie)) {
+        while (workerRunning && admin.getReadOnlyBookies().contains(targetBookie)) {
+            isInReadOnlyMode = true;
             waitBackOffTime();
         }
+
+        isInReadOnlyMode = false;
         LOG.info("Target bookie {} is back in read/write mode", targetBookie);
     }
 
@@ -451,6 +455,10 @@ boolean isRunning() {
         return workerRunning && workerThread.isAlive();
     }
 
+    boolean isInReadOnlyMode() {
+        return isInReadOnlyMode;
+    }
+
     private boolean isTargetBookieExistsInFragmentEnsemble(LedgerHandle lh,
             LedgerFragment ledgerFragment) {
         List<BookieSocketAddress> ensemble = ledgerFragment.getEnsemble();

diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java
@@ -506,10 +506,10 @@ public void testRWShouldReplicateTheLedgersAfterTimeoutIfLastFragmentIsNotUR()
     }
 
     /**
-     * Test that if the local bookie turns out to be readonly, then no point in running RW. So RW should shutdown.
+     * Test that if the local bookie turns out to be read-only, then the replicator will pause but not shutdown.
      */
     @Test(timeout = 20000)
-    public void testRWShutdownOnLocalBookieReadonlyTransition() throws Exception {
+    public void testRWOnLocalBookieReadonlyTransition() throws Exception {
         LedgerHandle lh = bkc.createLedger(3, 3, BookKeeper.DigestType.CRC32, TESTPASSWD);
 
         for (int i = 0; i < 10; i++) {
@@ -537,21 +537,22 @@ public void testRWShutdownOnLocalBookieReadonlyTransition() throws Exception {
             bsConfs.get(bsConfs.size() - 1).setReadOnlyModeEnabled(true);
             newBk.getBookie().doTransitionToReadOnlyMode();
             underReplicationManager.markLedgerUnderreplicated(lh.getId(), replicaToKill.toString());
-            while (ReplicationTestUtil.isLedgerInUnderReplication(zkc, lh.getId(), basePath) && rw.isRunning()) {
+            while (ReplicationTestUtil.isLedgerInUnderReplication(zkc, lh.getId(), basePath) && rw.isRunning()
+                    && !rw.isInReadOnlyMode()) {
                 Thread.sleep(100);
             }
             assertNull(zkc.exists(String.format("%s/urL%010d", baseLockPath, lh.getId()), false));
-            assertFalse("RW should shutdown if the bookie is readonly", rw.isRunning());
+            assertTrue("RW should continue even if the bookie is readonly", rw.isRunning());
         } finally {
             rw.shutdown();
         }
     }
 
     /**
-     * Test that the replication worker will shutdown if it lose its zookeeper session
+     * Test that the replication worker will not shutdown on a simple ZK disconnection
      */
     @Test(timeout=30000)
-    public void testRWZKSessionLost() throws Exception {
+    public void testRWZKConnectionLost() throws Exception {
         ZooKeeper zk = ZooKeeperClient.newBuilder()
                 .connectString(zkUtil.getZooKeeperConnectString())
                 .sessionTimeoutMs(10000)
@@ -567,15 +568,19 @@ public void testRWZKSessionLost() throws Exception {
                 Thread.sleep(1000);
             }
             assertTrue("Replication worker should be running", rw.isRunning());
-            stopZKCluster();
 
+            stopZKCluster();
+            // Wait for disconnection to be picked up
             for (int i = 0; i < 10; i++) {
-                if (!rw.isRunning()) {
+                if (!zk.getState().isConnected()) {
                     break;
                 }
                 Thread.sleep(1000);
             }
-            assertFalse("Replication worker should have shut down", rw.isRunning());
+            assertFalse(zk.getState().isConnected());
+            startZKCluster();
+
+            assertTrue("Replication worker should still be running", rw.isRunning());
         } finally {
             zk.close();
         }