You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@helix.apache.org by jx...@apache.org on 2020/04/23 01:59:44 UTC
[helix] branch master updated: Fix routing data refreshing in MSDS
(#955)
This is an automated email from the ASF dual-hosted git repository.
jxue pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git
The following commit(s) were added to refs/heads/master by this push:
new 9106e88 Fix routing data refreshing in MSDS (#955)
9106e88 is described below
commit 9106e884802a45722e7dc2068c3c51f2b730f90c
Author: Huizhi Lu <ih...@gmail.com>
AuthorDate: Wed Apr 22 18:59:38 2020 -0700
Fix routing data refreshing in MSDS (#955)
testSetNamespaceRoutingData is flaky because namespace is removed when refreshing routing data. It is caused by race condition between the read request after updating and data change callback to refresh routing. And routing data cache should not be refreshed if writing routing to ZK fails.
---
.../metadatastore/ZkMetadataStoreDirectory.java | 55 +++++++++++-----------
.../accessor/ZkRoutingDataReader.java | 1 -
.../apache/helix/rest/server/ServerContext.java | 1 -
.../server/resources/helix/ClusterAccessor.java | 5 +-
.../TestZkMetadataStoreDirectory.java | 11 +++--
5 files changed, 37 insertions(+), 36 deletions(-)
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 a869618..37395e1 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
@@ -175,9 +175,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
"Failed to set routing data: Namespace " + namespace + " is not found!");
}
synchronized (this) {
- boolean result = _routingDataWriterMap.get(namespace).setRoutingData(routingData);
+ if (!_routingDataWriterMap.get(namespace).setRoutingData(routingData)) {
+ return false;
+ }
refreshRoutingData(namespace);
- return result;
+ return true;
}
}
@@ -232,9 +234,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
"Failed to add metadata store realm: Namespace " + namespace + " is not found!");
}
synchronized (this) {
- boolean result = _routingDataWriterMap.get(namespace).addMetadataStoreRealm(realm);
+ if (!_routingDataWriterMap.get(namespace).addMetadataStoreRealm(realm)) {
+ return false;
+ }
refreshRoutingData(namespace);
- return result;
+ return true;
}
}
@@ -247,9 +251,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
"Failed to delete metadata store realm: Namespace " + namespace + " is not found!");
}
synchronized (this) {
- boolean result = _routingDataWriterMap.get(namespace).deleteMetadataStoreRealm(realm);
+ if (!_routingDataWriterMap.get(namespace).deleteMetadataStoreRealm(realm)) {
+ return false;
+ }
refreshRoutingData(namespace);
- return result;
+ return true;
}
}
@@ -272,9 +278,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
"Failed to add sharding key: Adding sharding key " + shardingKey
+ " makes routing data invalid!");
}
- boolean result = _routingDataWriterMap.get(namespace).addShardingKey(realm, shardingKey);
+ if (!_routingDataWriterMap.get(namespace).addShardingKey(realm, shardingKey)) {
+ return false;
+ }
refreshRoutingData(namespace);
- return result;
+ return true;
}
}
@@ -287,9 +295,11 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
"Failed to delete sharding key: Namespace " + namespace + " is not found!");
}
synchronized (this) {
- boolean result = _routingDataWriterMap.get(namespace).deleteShardingKey(realm, shardingKey);
+ if (!_routingDataWriterMap.get(namespace).deleteShardingKey(realm, shardingKey)) {
+ return false;
+ }
refreshRoutingData(namespace);
- return result;
+ return true;
}
}
@@ -304,16 +314,6 @@ public class ZkMetadataStoreDirectory implements MetadataStoreDirectory, Routing
*/
@Override
public void refreshRoutingData(String namespace) {
- // Safe to ignore the callback if any of the maps are null.
- // If routingDataMap is null, then it will be populated by the constructor anyway
- // If routingDataMap is not null, then it's safe for the callback function to update it
- if (_routingZkAddressMap == null || _realmToShardingKeysMap == null
- || _routingDataReaderMap == null || _routingDataWriterMap == null) {
- LOG.warn(
- "refreshRoutingData callback called before ZKMetadataStoreDirectory was fully initialized. Skipping refresh!");
- return;
- }
-
// Check if namespace exists; otherwise, return as a NOP and log it
if (!_routingZkAddressMap.containsKey(namespace)) {
LOG.error(
@@ -321,25 +321,26 @@ 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();
- _realmToShardingKeysMap.put(namespace, rawRoutingData);
} catch (InvalidRoutingDataException e) {
LOG.error("Failed to refresh cached routing data for namespace {}", namespace, e);
+ _realmToShardingKeysMap.put(namespace, Collections.emptyMap());
+ _routingDataMap.remove(namespace);
return;
}
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ TrieRoutingData trieRoutingData;
try {
- _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ trieRoutingData = new TrieRoutingData(rawRoutingData);
} catch (InvalidRoutingDataException e) {
LOG.warn("TrieRoutingData is not created for namespace {}", namespace, e);
+ _routingDataMap.remove(namespace);
+ return;
}
+ _routingDataMap.put(namespace, trieRoutingData);
}
@Override
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java b/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
index cfe6eb5..355cb82 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
@@ -29,7 +29,6 @@ import org.apache.helix.msdcommon.constant.MetadataStoreRoutingConstants;
import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory;
import org.apache.helix.zookeeper.api.client.HelixZkClient;
-import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
import org.apache.helix.zookeeper.datamodel.ZNRecord;
import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
import org.apache.helix.zookeeper.impl.factory.DedicatedZkClientFactory;
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java b/helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java
index c6632c2..86342f7 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/ServerContext.java
@@ -141,7 +141,6 @@ public class ServerContext implements IZkDataListener, IZkChildListener, IZkStat
.setZkSerializer(new ZNRecordSerializer()));
LOG.info("ServerContext: FederatedZkClient created successfully!");
} catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- LOG.error("Failed to create FederatedZkClient!", e);
throw new HelixException("Failed to create FederatedZkClient!", e);
}
} else {
diff --git a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
index 4ca775c..046e286 100644
--- a/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
+++ b/helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
@@ -174,9 +174,8 @@ public class ClusterAccessor extends AbstractHelixResource {
try {
clusterSetup.deleteCluster(clusterId);
} catch (HelixException ex) {
- LOG
- .info("Failed to delete cluster {}, cluster is still in use. Exception: {}.", clusterId,
- ex);
+ LOG.info("Failed to delete cluster {}, cluster is still in use. Exception: {}.", clusterId,
+ ex);
return badRequest(ex.getMessage());
} catch (Exception ex) {
LOG.error("Failed to delete cluster {}. Exception: {}.", clusterId, ex);
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 71e96ff..64e04df 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
@@ -170,7 +170,7 @@ public class TestZkMetadataStoreDirectory extends AbstractTestClass {
routingDataMap.put(TEST_REALM_2, TEST_SHARDING_KEYS_1);
for (String namespace : _routingZkAddrMap.keySet()) {
- _metadataStoreDirectory.setNamespaceRoutingData(namespace, routingDataMap);
+ Assert.assertTrue(_metadataStoreDirectory.setNamespaceRoutingData(namespace, routingDataMap));
Assert
.assertEquals(_metadataStoreDirectory.getNamespaceRoutingData(namespace), routingDataMap);
}
@@ -181,7 +181,8 @@ public class TestZkMetadataStoreDirectory extends AbstractTestClass {
originalRoutingDataMap.put(TEST_REALM_2, TEST_SHARDING_KEYS_2);
for (String namespace : _routingZkAddrMap.keySet()) {
- _metadataStoreDirectory.setNamespaceRoutingData(namespace, originalRoutingDataMap);
+ Assert.assertTrue(
+ _metadataStoreDirectory.setNamespaceRoutingData(namespace, originalRoutingDataMap));
Assert.assertEquals(_metadataStoreDirectory.getNamespaceRoutingData(namespace),
originalRoutingDataMap);
}
@@ -337,12 +338,14 @@ public class TestZkMetadataStoreDirectory extends AbstractTestClass {
Assert.assertTrue(TestHelper.verify(() -> {
for (String namespace : _routingZkAddrMap.keySet()) {
try {
- _metadataStoreDirectory.getMetadataStoreRealm(namespace, "anyKey");
+ _metadataStoreDirectory.getMetadataStoreRealm(namespace, TEST_SHARDING_KEYS_1.get(0));
+ // Metadata store realm is not yet refreshed. Retry.
return false;
} catch (IllegalStateException e) {
+ // If other IllegalStateException, it is unexpected and this test should fail.
if (!e.getMessage().equals("Failed to get metadata store realm: Namespace " + namespace
+ " contains either empty or invalid routing data!")) {
- return false;
+ throw e;
}
}
}