[GitHub] [helix] kaisun2000 commented on a change in pull request #642: Fix handleNewSession creating ephemeral node with expired session

[GitBox <gi...@apache.org>]

kaisun2000 commented on a change in pull request #642: Fix handleNewSession creating ephemeral node with expired session
URL: https://github.com/apache/helix/pull/642#discussion_r362605377
 
 

 ##########
 File path: helix-core/src/test/java/org/apache/helix/manager/zk/TestRawZkClient.java
 ##########
 @@ -461,4 +467,247 @@ void testPendingRequestGauge() throws Exception {
       zkServer.shutdown();
     }
   }
+
+  /*
+   * This test checks that a valid session can successfully create an ephemeral node.
+   */
+  @Test
+  public void testCreateEphemeralWithValidSession() throws Exception {
+    final String className = TestHelper.getTestClassName();
+    final String methodName = TestHelper.getTestMethodName();
+    final String clusterName = className + "_" + methodName;
+
+    TestHelper.setupCluster(clusterName, ZK_ADDR,
+        12918, // participant port
+        "localhost", // participant name prefix
+        "TestDB", // resource name prefix
+        1, // resources
+        10, // partitions per resource
+        5, // number of nodes
+        3, // replicas
+        "MasterSlave",
+        true); // do rebalance
+
+    final String originalSessionId = _zkClient.getHexSessionId();
+    final String path = "/" + methodName;
+    final String data = "Hello Helix";
+
+    // Verify the node is not existed yet.
+    Assert.assertFalse(_zkClient.exists(path));
+
+    // Wait until the ZkClient has got a new session.
+    Assert.assertTrue(TestHelper
+        .verify(() -> _zkClient.getConnection().getZookeeperState().isConnected(), 1000L));
+
+    try {
+      // Create ephemeral node.
+      _zkClient.createEphemeral(path, data, originalSessionId);
+    } catch (Exception ex) {
+      Assert.fail("Failed to create ephemeral node.", ex);
+    }
+
+    // Verify the node is created and its data is correct.
+    Stat stat = new Stat();
+    String nodeData = _zkClient.readData(path, stat, true);
+
+    Assert.assertNotNull(nodeData, "Failed to create ephemeral node: " + path);
+    Assert.assertEquals(nodeData, data, "Data is not correct.");
+    Assert.assertTrue(stat.getEphemeralOwner() != 0L,
+        "Ephemeral owner should NOT be zero because the node is an ephemeral node.");
+    Assert.assertEquals(ZKUtil.toHexSessionId(stat.getEphemeralOwner()), originalSessionId,
+        "Ephemeral node is created by an unexpected session");
+
+    // Delete the node to clean up, otherwise, the ephemeral node would be existed
+    // until the session of its owner expires.
+    _zkClient.delete(path);
+  }
+
+  /*
+   * This test checks that ephemeral creation fails because the expected zk session does not match
+   * the actual zk session.
+   * How this test does is:
+   * 1. Creates a zk client and gets its original session id
+   * 2. Expires the original session, and a new session will be created
+   * 3. Tries to create ephemeral node with the original session
+   * 4. ZkSessionMismatchedException is expected for the creation and ephemeral node is not created
+   */
+  @Test
+  public void testCreateEphemeralWithMismatchedSession() throws Exception {
+    final String className = TestHelper.getTestClassName();
+    final String methodName = TestHelper.getTestMethodName();
+    final String clusterName = className + "_" + methodName;
+
+    TestHelper.setupCluster(clusterName, ZK_ADDR,
+        12918, // participant port
+        "localhost", // participant name prefix
+        "TestDB", // resource name prefix
+        1, // resources
+        10, // partitions per resource
+        5, // number of nodes
+        3, // replicas
+        "MasterSlave",
+        true); // do rebalance
+
+    final long originalSessionId = _zkClient.getSessionId();
+    final String originalHexSessionId = _zkClient.getHexSessionId();
+    final String path = "/" + methodName;
+
+    // Verify the node is not existed.
+    Assert.assertFalse(_zkClient.exists(path));
+
+    // Expire the original session.
+    ZkTestHelper.expireSession(_zkClient);
+
+    // Wait until the ZkClient has got a new session.
+    Assert.assertTrue(TestHelper.verify(() -> {
+      try {
+        // New session id should be greater than expired session id.
+        return _zkClient.getSessionId() > originalSessionId;
 
 Review comment:
   Nit, !=  should be ok. Zookeeper does not really guarantee >. Maybe later version they may change it.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org