[helix] 50/50: Fix MetadataStoreDirectory routing data cache refresh bug (#933)

[hu...@apache.org]

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

hulee pushed a commit to branch zooscalability_merge
in repository https://gitbox.apache.org/repos/asf/helix.git

commit 973009b8f5eed4d4331d733f7550429b1e1c77c4
Author: Neal Sun <ne...@gmail.com>
AuthorDate: Tue Apr 7 18:03:50 2020 -0700

    Fix MetadataStoreDirectory routing data cache refresh bug (#933)
    
    This PR ensures that routing data cache is cleared before updating in ZkMetadataStoreDirectory. Also, this PR fixes an edge case during TrieRoutingData creation when no zk realms has any sharding key.
---
 .../metadatastore/ZkMetadataStoreDirectory.java    |  5 ++++
 .../TestZkMetadataStoreDirectory.java              | 31 ++++++++++++++++++++++
 .../helix/msdcommon/datamodel/TrieRoutingData.java | 19 +++++++++++++
 .../msdcommon/datamodel/TestTrieRoutingData.java   |  8 ++++++
 4 files changed, 63 insertions(+)

diff --git a/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java b/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
index 42b2b17..a869618 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
@@ -321,6 +321,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
       return;
     }
 
+    // Remove the raw data first in case of failure on creation
+    _realmToShardingKeysMap.remove(namespace);
+    // Remove routing data first in case of failure on creation
+    _routingDataMap.remove(namespace);
+
     Map<String, List<String>> rawRoutingData;
     try {
       rawRoutingData = _routingDataReaderMap.get(namespace).getRoutingData();
diff --git a/helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java b/helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
index 47d6fba..71e96ff 100644
--- a/helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
+++ b/helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
@@ -319,6 +319,37 @@ public class TestZkMetadataStoreDirectory extends AbstractTestClass {
     }, TestHelper.WAIT_DURATION));
   }
 
+  @Test(dependsOnMethods = "testChildChangeCallback")
+  public void testDataDeletionCallback() throws Exception {
+    // For all namespaces, delete all routing data
+    _zkList.forEach(zk -> {
+        ZkClient zkClient = ZK_SERVER_MAP.get(zk).getZkClient();
+        if (zkClient.exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
+          for (String zkRealm : zkClient
+              .getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
+            zkClient.delete(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm);
+          }
+        }
+      }
+    );
+
+    // Confirm that TrieRoutingData has been removed due to missing routing data
+    Assert.assertTrue(TestHelper.verify(() -> {
+      for (String namespace : _routingZkAddrMap.keySet()) {
+        try {
+          _metadataStoreDirectory.getMetadataStoreRealm(namespace, "anyKey");
+          return false;
+        } catch (IllegalStateException e) {
+          if (!e.getMessage().equals("Failed to get metadata store realm: Namespace " + namespace
+              + " contains either empty or invalid routing data!")) {
+            return false;
+          }
+        }
+      }
+      return true;
+    }, TestHelper.WAIT_DURATION));
+  }
+
   private void clearRoutingData() throws Exception {
     Assert.assertTrue(TestHelper.verify(() -> {
       for (String zk : _zkList) {
diff --git a/metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java b/metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java
index 94bb331..b174357 100644
--- a/metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java
+++ b/metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java
@@ -48,6 +48,10 @@ public class TrieRoutingData implements MetadataStoreRoutingData {
       throw new InvalidRoutingDataException("routingData cannot be null or empty");
     }
 
+    if (!containsShardingKey(routingData)) {
+      throw new InvalidRoutingDataException("routingData needs at least 1 sharding key");
+    }
+
     if (isRootShardingKey(routingData)) {
       Map.Entry<String, List<String>> entry = routingData.entrySet().iterator().next();
       _rootNode = new TrieNode(Collections.emptyMap(), "/", true, entry.getKey());
@@ -167,6 +171,21 @@ public class TrieRoutingData implements MetadataStoreRoutingData {
   }
 
   /*
+   * Checks if there is any sharding key in the routing data
+   * @param routingData - a mapping from "sharding keys" to "realm addresses" to be parsed into a
+   *          trie
+   * @return whether there is any sharding key
+   */
+  private boolean containsShardingKey(Map<String, List<String>> routingData) {
+    for (Map.Entry<String, List<String>> entry : routingData.entrySet()) {
+      if (entry.getValue().size() > 0) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /*
    * Checks for the edge case when the only sharding key in provided routing data is the delimiter.
    * When this is the case, the trie is valid and contains only one node, which
    * is the root node, and the root node is a leaf node with a realm address associated with it.
diff --git a/metadata-store-directory-common/src/test/java/org/apache/helix/msdcommon/datamodel/TestTrieRoutingData.java b/metadata-store-directory-common/src/test/java/org/apache/helix/msdcommon/datamodel/TestTrieRoutingData.java
index 2dc5dfe..44b7e6e 100644
--- a/metadata-store-directory-common/src/test/java/org/apache/helix/msdcommon/datamodel/TestTrieRoutingData.java
+++ b/metadata-store-directory-common/src/test/java/org/apache/helix/msdcommon/datamodel/TestTrieRoutingData.java
@@ -48,6 +48,14 @@ public class TestTrieRoutingData {
     } catch (InvalidRoutingDataException e) {
       Assert.assertTrue(e.getMessage().contains("routingData cannot be null or empty"));
     }
+    Map<String, List<String>> routingData = new HashMap<>();
+    routingData.put("realmAddress", Collections.emptyList());
+    try {
+      new TrieRoutingData(routingData);
+      Assert.fail("Expecting InvalidRoutingDataException");
+    } catch (InvalidRoutingDataException e) {
+      Assert.assertTrue(e.getMessage().contains("routingData needs at least 1 sharding key"));
+    }
   }
 
   /**