From 8c125a40bf92b19e1062d058c653ef7d6e5200f4 Mon Sep 17 00:00:00 2001
From: stack <stack@apache.org>
Date: Fri, 10 Oct 2014 10:28:16 -0700
Subject: [PATCH] HBASE-11394 Replication can have data loss if peer id
 contains hyphen "-"

---
 .../replication/ReplicationPeersZKImpl.java   |  5 +++
 .../apache/hadoop/hbase/zookeeper/ZKUtil.java |  6 +++-
 .../TestReplicationTrackerZKImpl.java         | 31 +++++++++++++++++--
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeersZKImpl.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeersZKImpl.java
index abb0984ef67..cc49a640a07 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeersZKImpl.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/replication/ReplicationPeersZKImpl.java
@@ -110,6 +110,11 @@ public class ReplicationPeersZKImpl extends ReplicationStateZKBase implements Re
         throw new IllegalArgumentException("Cannot add a peer with id=" + id
             + " because that id already exists.");
       }
+      
+      if(id.contains("-")){
+        throw new IllegalArgumentException("Found invalid peer name:" + id);
+      }
+      
       ZKUtil.createWithParents(this.zookeeper, this.peersZNode);
       List<ZKUtilOp> listOfOps = new ArrayList<ZKUtil.ZKUtilOp>();
       ZKUtilOp op1 = ZKUtilOp.createAndFailSilent(ZKUtil.joinZNode(this.peersZNode, id),
diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
index d3c39e5b3c2..eca72a8670f 100644
--- a/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
+++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
@@ -1321,7 +1321,11 @@ public class ZKUtil {
           deleteNodeRecursively(zkw, joinZNode(node, child));
         }
       }
-      zkw.getRecoverableZooKeeper().delete(node, -1);
+      //Zookeeper Watches are one time triggers; When children of parent nodes are deleted recursively. 
+      //Must set another watch, get notified of delete node   
+      if (zkw.getRecoverableZooKeeper().exists(node, zkw) != null){
+        zkw.getRecoverableZooKeeper().delete(node, -1);
+      }
     } catch(InterruptedException ie) {
       zkw.interruptedException(ie);
     }
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationTrackerZKImpl.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationTrackerZKImpl.java
index 1c3de71d741..55de56579f0 100644
--- a/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationTrackerZKImpl.java
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationTrackerZKImpl.java
@@ -143,7 +143,7 @@ public class TestReplicationTrackerZKImpl {
     assertEquals("hostname2.example.org:1234", rsRemovedData);
   }
 
-  @Ignore ("Flakey") @Test(timeout = 30000)
+  @Test(timeout = 30000)
   public void testPeerRemovedEvent() throws Exception {
     rp.addPeer("5", new ReplicationPeerConfig().setClusterKey(utility.getClusterKey()), null);
     rt.registerListener(new DummyReplicationListener());
@@ -155,7 +155,7 @@ public class TestReplicationTrackerZKImpl {
     assertEquals("5", peerRemovedData);
   }
 
-  @Ignore ("Flakey") @Test(timeout = 30000)
+  @Test(timeout = 30000)
   public void testPeerListChangedEvent() throws Exception {
     // add a peer
     rp.addPeer("5", new ReplicationPeerConfig().setClusterKey(utility.getClusterKey()), null);
@@ -172,9 +172,34 @@ public class TestReplicationTrackerZKImpl {
     assertTrue(plChangedData.contains("5"));
 
     // clean up
-    ZKUtil.deleteNode(zkw, "/hbase/replication/peers/5");
+    //ZKUtil.deleteNode(zkw, "/hbase/replication/peers/5");
+    rp.removePeer("5");
   }
 
+  @Test(timeout = 30000)
+  public void testPeerNameControl() throws Exception {
+    int exists = 0;
+    int hyphen = 0;
+    rp.addPeer("6", new ReplicationPeerConfig().setClusterKey(utility.getClusterKey()), null);
+    
+    try{
+      rp.addPeer("6", new ReplicationPeerConfig().setClusterKey(utility.getClusterKey()), null);
+    }catch(IllegalArgumentException e){
+      exists++;
+    }
+
+    try{
+      rp.addPeer("6-ec2", new ReplicationPeerConfig().setClusterKey(utility.getClusterKey()), null);
+    }catch(IllegalArgumentException e){
+      hyphen++;
+    }
+    assertEquals(1, exists);
+    assertEquals(1, hyphen);
+    
+    // clean up
+    rp.removePeer("6");
+  }
+  
   private class DummyReplicationListener implements ReplicationListener {
 
     @Override