You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/02 23:14:15 UTC
[GitHub] [helix] NealSun96 opened a new pull request #844: Implement
setRoutingData for MetadataStoreDirectoryService
NealSun96 opened a new pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #841, #842, #843
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This PR implements `setRoutingData`, which was an endpoint that was defined but not finished. It allows users to overwrite whatever routing data there is for a namespace and replace it with a new set of data. This endpoint was designed with convenience in mind because it allows users to specify the routing data in one call instead of making multiple calls to create zkrealms and add sharding keys. This endpoint was originally designed to validate the input but now it doesn't, please refer #842 for the reasoning.
Related to this PR, we also discovered that there are problems related to the construction of `TrieRoutingData` in `MetadataStoreDirectory`. Please see #841 .
Lastly, we are also fixing the race conditions of writing operations in `MetadataStoreDirectory` which are discovered during the development. Please see #843.
### Tests
- [x] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 140, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 24.602 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 140, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 53.115 s
[INFO] Finished at: 2020-03-02T14:38:49-08:00
[INFO] ------------------------------------------------------------------------
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386766157
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -262,17 +309,22 @@ public void refreshRoutingData(String namespace) {
if (!_routingZkAddressMap.containsKey(namespace)) {
LOG.error(
"Failed to refresh internally-cached routing data! Namespace not found: " + namespace);
+ return;
}
+ Map<String, List<String>> rawRoutingData;
try {
- Map<String, List<String>> rawRoutingData =
- _routingDataReaderMap.get(namespace).getRoutingData();
+ rawRoutingData = _routingDataReaderMap.get(namespace).getRoutingData();
_realmToShardingKeysMap.put(namespace, rawRoutingData);
-
- MetadataStoreRoutingData routingData = new TrieRoutingData(rawRoutingData);
- _routingDataMap.put(namespace, routingData);
} catch (InvalidRoutingDataException e) {
LOG.error("Failed to refresh cached routing data for namespace {}", namespace, e);
+ return;
+ }
+
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
Probably a good idea to add a warn log here?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387230828
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##########
@@ -220,6 +228,24 @@ public Response getRoutingData() {
return JSONRepresentation(responseMap);
}
+ @PUT
+ @Path("/routing-data")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response setRoutingData(String jsonContent) {
+ try {
+ Map<String, List<String>> routingData = new ObjectMapper()
Review comment:
On a second thought, the ObjectMapper will be static and is reusable. Making the changes, then.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386765530
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -169,19 +186,31 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get all mapping under path: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get all mapping under path: Namespace " + namespace
+ + " contains invalid routing data!");
+ }
return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get metadata store realm: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get metadata store realm: Namespace " + namespace
+ + " contains invalid routing data!");
Review comment:
Same as above :)
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #844: Implement
setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386716899
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -169,72 +186,102 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get all mapping under path: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get all mapping under path: Namespace " + namespace
+ + " contains invalid routing data!");
+ }
return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get metadata store realm: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get metadata store realm: Namespace " + namespace
+ + " contains invalid routing data!");
+ }
return _routingDataMap.get(namespace).getMetadataStoreRealm(shardingKey);
}
@Override
public boolean addMetadataStoreRealm(String namespace, String realm) {
- if (!_routingDataWriterMap.containsKey(namespace)) {
- // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
- // status code in the Accessor level
- throw new NoSuchElementException(
- "Failed to add metadata store realm: Namespace " + namespace + " is not found!");
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
Review comment:
Before synchronized
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387192602
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -209,8 +214,20 @@ public synchronized boolean setRoutingData(Map<String, List<String>> routingData
return true;
}
- // TODO: Forward the request to leader
- return true;
+ String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
+ String url = leaderHostName + constructUrlSuffix(
+ MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+ HttpPut httpPut = new HttpPut(url);
+ String routingDataJsonString;
+ try {
+ routingDataJsonString = new ObjectMapper().writeValueAsString(routingData);
Review comment:
Sure.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386766561
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -209,8 +214,20 @@ public synchronized boolean setRoutingData(Map<String, List<String>> routingData
return true;
}
- // TODO: Forward the request to leader
- return true;
+ String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
+ String url = leaderHostName + constructUrlSuffix(
+ MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+ HttpPut httpPut = new HttpPut(url);
+ String routingDataJsonString;
+ try {
+ routingDataJsonString = new ObjectMapper().writeValueAsString(routingData);
Review comment:
Could we create a constant variable ObjectMapper instead of creating one every time this endpoint is called?
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
and you could use this throughout the class :)
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386764843
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -113,10 +113,14 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
_routingDataWriterMap.put(namespace, new ZkRoutingDataWriter(namespace, zkAddress));
// Populate realmToShardingKeys with ZkRoutingDataReader
- _realmToShardingKeysMap
- .put(namespace, _routingDataReaderMap.get(namespace).getRoutingData());
- _routingDataMap
- .put(namespace, new TrieRoutingData(_realmToShardingKeysMap.get(namespace)));
+ Map<String, List<String>> rawRoutingData =
+ _routingDataReaderMap.get(namespace).getRoutingData();
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
I think a WARN log would be okay to add here. Note that we are not expecting very frequent writes :)
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387233329
##########
File path: metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java
##########
@@ -167,8 +167,8 @@ private TrieNode getLongestPrefixNodeAlongPath(String path) {
}
/*
- * Checks for the edge case when the only sharding key in provided routing data is the delimiter
- * or an empty string. When this is the case, the trie is valid and contains only one node, which
+ * 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
Review comment:
The comment change here was regarding to the sharding key being either "/" or "", not routing data being empty. I believe your question is about when the entire routing data is empty: the answer is yes, we throw an exception. That behavior, as we agreed on, will not be changed.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386766805
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -332,8 +349,9 @@ private String constructUrlSuffix(String... urlParams) {
return String.join("", allUrlParameters);
}
- private boolean forwardRequestToLeader(String urlSuffix, HttpConstants.RestVerbs request_method,
- int expectedResponseCode) throws IllegalArgumentException {
+ private boolean buildAndSendRequestToLeader(String urlSuffix,
+ HttpConstants.RestVerbs request_method, int expectedResponseCode)
Review comment:
shouldWeUseCamelCaseFor `request_method`?
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #844: Implement
setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386715625
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -155,6 +159,19 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
return routingData;
}
+ @Override
+ public boolean setNamespaceRoutingData(String namespace, Map<String, List<String>> routingData) {
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
Review comment:
This is like double lock checking. You may also want to check it before synchronized which also improves performance.
Considering namespace is not changed, you don’t have to check it in within synchronized block.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386767172
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##########
@@ -220,6 +228,24 @@ public Response getRoutingData() {
return JSONRepresentation(responseMap);
}
+ @PUT
+ @Path("/routing-data")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response setRoutingData(String jsonContent) {
+ try {
+ Map<String, List<String>> routingData = new ObjectMapper()
Review comment:
Same thing as above. We can create ObjectMapper once and use it over and over.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387202542
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/MetadataStoreDirectoryAccessorTestBase.java
##########
@@ -100,23 +100,19 @@ public void beforeClass() throws Exception {
@AfterClass
public void afterClass() throws Exception {
System.clearProperty(MetadataStoreRoutingConstants.MSDS_SERVER_HOSTNAME_KEY);
+ _routingDataReader.close();
deleteRoutingDataPath();
}
- protected void deleteRoutingDataPath() throws Exception {
- Assert.assertTrue(TestHelper.verify(() -> {
- _zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
-
- for (String zk : _zkList) {
- if (ZK_SERVER_MAP.get(zk).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
+ protected void deleteRoutingDataPath() {
+ for (String zk : _zkList) {
+ 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);
Review comment:
Very good point. I'll try to add the same block to all similar clean ups.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387192864
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -332,8 +349,9 @@ private String constructUrlSuffix(String... urlParams) {
return String.join("", allUrlParameters);
}
- private boolean forwardRequestToLeader(String urlSuffix, HttpConstants.RestVerbs request_method,
- int expectedResponseCode) throws IllegalArgumentException {
+ private boolean buildAndSendRequestToLeader(String urlSuffix,
+ HttpConstants.RestVerbs request_method, int expectedResponseCode)
Review comment:
Ah sorry, python habits coming through. Fixing 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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387214326
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +116,11 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
- Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
- }
-
- return true;
- }, TestHelper.WAIT_DURATION), "Routing data path should be deleted after the tests.");
+ private void clearRoutingDataPath() {
+ for (String zkRealm : _baseAccessor
+ .getChildNames(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, AccessOption.PERSISTENT)) {
+ _baseAccessor.remove(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm,
+ AccessOption.PERSISTENT);
Review comment:
Resolved offline. Using `HelixZkClient` and will migrate later.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386768683
##########
File path: metadata-store-directory-common/src/main/java/org/apache/helix/msdcommon/datamodel/TrieRoutingData.java
##########
@@ -167,8 +167,8 @@ private TrieNode getLongestPrefixNodeAlongPath(String path) {
}
/*
- * Checks for the edge case when the only sharding key in provided routing data is the delimiter
- * or an empty string. When this is the case, the trie is valid and contains only one node, which
+ * 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
Review comment:
Question: we still want to throw an exception when the data is empty, right?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387323377
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -332,20 +350,21 @@ private String constructUrlSuffix(String... urlParams) {
return String.join("", allUrlParameters);
}
- private boolean forwardRequestToLeader(String urlSuffix, HttpConstants.RestVerbs request_method,
- int expectedResponseCode) throws IllegalArgumentException {
+ private boolean buildAndSendRequestToLeader(String urlSuffix,
+ HttpConstants.RestVerbs requestMethod, int expectedResponseCode)
+ throws IllegalArgumentException {
String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
String url = leaderHostName + urlSuffix;
HttpUriRequest request;
- switch (request_method) {
+ switch (requestMethod) {
case PUT:
request = new HttpPut(url);
break;
case DELETE:
request = new HttpDelete(url);
break;
default:
- LOG.error("Unsupported request_method: " + request_method.name());
+ LOG.error("Unsupported requestMethod: " + requestMethod.name());
Review comment:
This is a strange one because it can only go to `default` because of developer oversights.
If some code using the wrong http verb does make it to production, and clients are calling the endpoints with the incorrect code, we definitely want to report 500 to clients (return false) instead of 400 (`IllegalArgumentException`).
I'll change it to exception because `return false` isn't informative enough to developers, but I wish there's a way to limit the values of a parameter.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387324208
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
##########
@@ -277,4 +315,27 @@ public void testChildChangeCallback() throws Exception {
return false;
}, TestHelper.WAIT_DURATION));
}
+
+ private void clearRoutingData() throws Exception {
+ Assert.assertTrue(TestHelper.verify(() -> {
+ for (String zk : _zkList) {
Review comment:
For your convenience:
```
public static boolean verify(Verifier verifier, long timeout) throws Exception {
long start = System.currentTimeMillis();
do {
boolean result = verifier.verify();
if (result || (System.currentTimeMillis() - start) > timeout) {
return result;
}
Thread.sleep(50);
} while (true);
}
```
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387190921
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -113,10 +113,14 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
_routingDataWriterMap.put(namespace, new ZkRoutingDataWriter(namespace, zkAddress));
// Populate realmToShardingKeys with ZkRoutingDataReader
- _realmToShardingKeysMap
- .put(namespace, _routingDataReaderMap.get(namespace).getRoutingData());
- _routingDataMap
- .put(namespace, new TrieRoutingData(_realmToShardingKeysMap.get(namespace)));
+ Map<String, List<String>> rawRoutingData =
+ _routingDataReaderMap.get(namespace).getRoutingData();
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
Using WARN is a good middle ground. I'll add logging for this section and also the next. Thank you for the suggestion!
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #844: Implement
setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386714466
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -113,10 +113,14 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
_routingDataWriterMap.put(namespace, new ZkRoutingDataWriter(namespace, zkAddress));
// Populate realmToShardingKeys with ZkRoutingDataReader
- _realmToShardingKeysMap
- .put(namespace, _routingDataReaderMap.get(namespace).getRoutingData());
- _routingDataMap
- .put(namespace, new TrieRoutingData(_realmToShardingKeysMap.get(namespace)));
+ Map<String, List<String>> rawRoutingData =
+ _routingDataReaderMap.get(namespace).getRoutingData();
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
Do we want to throw any exception or log this?
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387319964
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -209,8 +215,20 @@ public synchronized boolean setRoutingData(Map<String, List<String>> routingData
return true;
}
- // TODO: Forward the request to leader
- return true;
+ String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
+ String url = leaderHostName + constructUrlSuffix(
+ MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+ HttpPut httpPut = new HttpPut(url);
+ String routingDataJsonString;
+ try {
+ routingDataJsonString = OBJECT_MAPPER.writeValueAsString(routingData);
+ } catch (JsonGenerationException | JsonMappingException e) {
+ throw new IllegalArgumentException(e.getMessage());
+ } catch (IOException e) {
+ return false;
+ }
Review comment:
Sure. This block should technically never have a problem though. I'll add a log.
----------------------------------------------------------------
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
[GitHub] [helix] pkuwm commented on a change in pull request #844: Implement
setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386716142
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -169,72 +186,102 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get all mapping under path: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get all mapping under path: Namespace " + namespace
+ + " contains invalid routing data!");
+ }
return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get metadata store realm: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get metadata store realm: Namespace " + namespace
+ + " contains invalid routing data!");
+ }
return _routingDataMap.get(namespace).getMetadataStoreRealm(shardingKey);
}
@Override
public boolean addMetadataStoreRealm(String namespace, String realm) {
- if (!_routingDataWriterMap.containsKey(namespace)) {
- // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
- // status code in the Accessor level
- throw new NoSuchElementException(
- "Failed to add metadata store realm: Namespace " + namespace + " is not found!");
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
+ // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
+ // status code in the Accessor level
+ throw new NoSuchElementException(
+ "Failed to add metadata store realm: Namespace " + namespace + " is not found!");
+ }
+ boolean result = _routingDataWriterMap.get(namespace).addMetadataStoreRealm(realm);
+ refreshRoutingData(namespace);
+ return result;
}
- return _routingDataWriterMap.get(namespace).addMetadataStoreRealm(realm);
}
@Override
public boolean deleteMetadataStoreRealm(String namespace, String realm) {
- if (!_routingDataWriterMap.containsKey(namespace)) {
- // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
- // status code in the Accessor level
- throw new NoSuchElementException(
- "Failed to delete metadata store realm: Namespace " + namespace + " is not found!");
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
+ // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
+ // status code in the Accessor level
+ throw new NoSuchElementException(
+ "Failed to delete metadata store realm: Namespace " + namespace + " is not found!");
+ }
+ boolean result = _routingDataWriterMap.get(namespace).deleteMetadataStoreRealm(realm);
+ refreshRoutingData(namespace);
+ return result;
}
- return _routingDataWriterMap.get(namespace).deleteMetadataStoreRealm(realm);
}
@Override
public boolean addShardingKey(String namespace, String realm, String shardingKey) {
- if (!_routingDataWriterMap.containsKey(namespace) || !_routingDataMap.containsKey(namespace)) {
- // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
- // status code in the Accessor level
- throw new NoSuchElementException(
- "Failed to add sharding key: Namespace " + namespace + " is not found!");
- }
- if (_routingDataMap.get(namespace).containsKeyRealmPair(shardingKey, realm)) {
- return true;
- }
- if (!_routingDataMap.get(namespace).isShardingKeyInsertionValid(shardingKey)) {
- throw new IllegalArgumentException(
- "Failed to add sharding key: Adding sharding key " + shardingKey
- + " makes routing data invalid!");
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
+ // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
+ // status code in the Accessor level
+ throw new NoSuchElementException(
+ "Failed to add sharding key: Namespace " + namespace + " is not found!");
+ }
+ if (_routingDataMap.containsKey(namespace) && _routingDataMap.get(namespace)
+ .containsKeyRealmPair(shardingKey, realm)) {
+ return true;
+ }
+ if (_routingDataMap.containsKey(namespace) && !_routingDataMap.get(namespace)
+ .isShardingKeyInsertionValid(shardingKey)) {
+ throw new IllegalArgumentException(
+ "Failed to add sharding key: Adding sharding key " + shardingKey
+ + " makes routing data invalid!");
+ }
+ boolean result = _routingDataWriterMap.get(namespace).addShardingKey(realm, shardingKey);
+ refreshRoutingData(namespace);
+ return result;
}
- return _routingDataWriterMap.get(namespace).addShardingKey(realm, shardingKey);
}
@Override
public boolean deleteShardingKey(String namespace, String realm, String shardingKey) {
- if (!_routingDataWriterMap.containsKey(namespace)) {
- // throwing NoSuchElementException instead of IllegalArgumentException to differentiate the
- // status code in the Accessor level
- throw new NoSuchElementException(
- "Failed to delete sharding key: Namespace " + namespace + " is not found!");
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
Review comment:
move it before synchronized
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387317979
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +123,14 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
+ private void clearRoutingDataPath() throws Exception {
Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
+ for (String zkRealm : _zkClient
+ .getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
+ _zkClient.delete(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm);
Review comment:
Ditto above.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387274120
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -209,8 +215,20 @@ public synchronized boolean setRoutingData(Map<String, List<String>> routingData
return true;
}
- // TODO: Forward the request to leader
- return true;
+ String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
+ String url = leaderHostName + constructUrlSuffix(
+ MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+ HttpPut httpPut = new HttpPut(url);
+ String routingDataJsonString;
+ try {
+ routingDataJsonString = OBJECT_MAPPER.writeValueAsString(routingData);
+ } catch (JsonGenerationException | JsonMappingException e) {
+ throw new IllegalArgumentException(e.getMessage());
+ } catch (IOException e) {
+ return false;
+ }
Review comment:
Do we need to log the exception here? Otherwise you'd be failing silently.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387274622
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -332,20 +350,21 @@ private String constructUrlSuffix(String... urlParams) {
return String.join("", allUrlParameters);
}
- private boolean forwardRequestToLeader(String urlSuffix, HttpConstants.RestVerbs request_method,
- int expectedResponseCode) throws IllegalArgumentException {
+ private boolean buildAndSendRequestToLeader(String urlSuffix,
+ HttpConstants.RestVerbs requestMethod, int expectedResponseCode)
+ throws IllegalArgumentException {
String leaderHostName = _leaderElection.getCurrentLeaderInfo().getId();
String url = leaderHostName + urlSuffix;
HttpUriRequest request;
- switch (request_method) {
+ switch (requestMethod) {
case PUT:
request = new HttpPut(url);
break;
case DELETE:
request = new HttpDelete(url);
break;
default:
- LOG.error("Unsupported request_method: " + request_method.name());
+ LOG.error("Unsupported requestMethod: " + requestMethod.name());
Review comment:
I think it would be better to throw an exception here because this is not supposed to happen.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386767904
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataWriter.java
##########
@@ -132,61 +132,86 @@ public void testSetRoutingData() {
public void testAddMetadataStoreRealmNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.addMetadataStoreRealm(DUMMY_REALM);
- Assert.assertEquals("PUT", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "PUT");
List<String> expectedUrlParams = Arrays
.asList(MetadataStoreRoutingConstants.MSDS_NAMESPACES_URL_PREFIX, DUMMY_NAMESPACE,
MetadataStoreRoutingConstants.MSDS_GET_ALL_REALMS_ENDPOINT, DUMMY_REALM);
String expectedUrl =
getBaseUri().toString() + String.join("/", expectedUrlParams).replaceAll("//", "/")
.substring(1);
- Assert.assertEquals(expectedUrl, mockWriter.calledRequest.getURI().toString());
+ Assert.assertEquals(mockWriter.calledRequest.getURI().toString(), expectedUrl);
mockWriter.close();
}
@Test(dependsOnMethods = "testAddMetadataStoreRealmNonLeader")
public void testDeleteMetadataStoreRealmNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.deleteMetadataStoreRealm(DUMMY_REALM);
- Assert.assertEquals("DELETE", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "DELETE");
List<String> expectedUrlParams = Arrays
.asList(MetadataStoreRoutingConstants.MSDS_NAMESPACES_URL_PREFIX, DUMMY_NAMESPACE,
MetadataStoreRoutingConstants.MSDS_GET_ALL_REALMS_ENDPOINT, DUMMY_REALM);
String expectedUrl =
getBaseUri().toString() + String.join("/", expectedUrlParams).replaceAll("//", "/")
.substring(1);
- Assert.assertEquals(expectedUrl, mockWriter.calledRequest.getURI().toString());
+ Assert.assertEquals(mockWriter.calledRequest.getURI().toString(), expectedUrl);
mockWriter.close();
}
@Test(dependsOnMethods = "testDeleteMetadataStoreRealmNonLeader")
public void testAddShardingKeyNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.addShardingKey(DUMMY_REALM, DUMMY_SHARDING_KEY);
- Assert.assertEquals("PUT", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "PUT");
List<String> expectedUrlParams = Arrays
.asList(MetadataStoreRoutingConstants.MSDS_NAMESPACES_URL_PREFIX, DUMMY_NAMESPACE,
MetadataStoreRoutingConstants.MSDS_GET_ALL_REALMS_ENDPOINT, DUMMY_REALM,
MetadataStoreRoutingConstants.MSDS_GET_ALL_SHARDING_KEYS_ENDPOINT, DUMMY_SHARDING_KEY);
String expectedUrl =
getBaseUri().toString() + String.join("/", expectedUrlParams).replaceAll("//", "/")
.substring(1);
- Assert.assertEquals(expectedUrl, mockWriter.calledRequest.getURI().toString());
+ Assert.assertEquals(mockWriter.calledRequest.getURI().toString(), expectedUrl);
mockWriter.close();
}
@Test(dependsOnMethods = "testAddShardingKeyNonLeader")
public void testDeleteShardingKeyNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.deleteShardingKey(DUMMY_REALM, DUMMY_SHARDING_KEY);
- Assert.assertEquals("DELETE", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "DELETE");
List<String> expectedUrlParams = Arrays
.asList(MetadataStoreRoutingConstants.MSDS_NAMESPACES_URL_PREFIX, DUMMY_NAMESPACE,
MetadataStoreRoutingConstants.MSDS_GET_ALL_REALMS_ENDPOINT, DUMMY_REALM,
MetadataStoreRoutingConstants.MSDS_GET_ALL_SHARDING_KEYS_ENDPOINT, DUMMY_SHARDING_KEY);
String expectedUrl =
getBaseUri().toString() + String.join("/", expectedUrlParams).replaceAll("//", "/")
.substring(1);
- Assert.assertEquals(expectedUrl, mockWriter.calledRequest.getURI().toString());
+ Assert.assertEquals(mockWriter.calledRequest.getURI().toString(), expectedUrl);
mockWriter.close();
}
+
+ @Test(dependsOnMethods = "testDeleteShardingKeyNonLeader")
+ public void testSetRoutingDataNonLeader() {
+ MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
+ Map<String, List<String>> testRoutingDataMap =
+ ImmutableMap.of(DUMMY_REALM, Collections.singletonList(DUMMY_SHARDING_KEY));
+ mockWriter.setRoutingData(testRoutingDataMap);
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "PUT");
+ List<String> expectedUrlParams = Arrays
+ .asList(MetadataStoreRoutingConstants.MSDS_NAMESPACES_URL_PREFIX, DUMMY_NAMESPACE,
+ MetadataStoreRoutingConstants.MSDS_GET_ALL_ROUTING_DATA_ENDPOINT);
+ String expectedUrl =
+ getBaseUri().toString() + String.join("/", expectedUrlParams).replaceAll("//", "/")
+ .substring(1);
+ Assert.assertEquals(mockWriter.calledRequest.getURI().toString(), expectedUrl);
+ mockWriter.close();
+ }
+
+ private void clearRoutingDataPath() {
+ for (String zkRealm : _baseAccessor
+ .getChildNames(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, AccessOption.PERSISTENT)) {
+ _baseAccessor.remove(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm,
+ AccessOption.PERSISTENT);
Review comment:
It would be better to stick with ZkClient if possible outside of helix-core module.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on issue #844: Implement setRoutingData
for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on issue #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#issuecomment-594230671
This PR is ready to be merged, approved by @narendly
Final commit message:
## Implement setRoutingData for MetadataStoreDirectoryService ##
Implement setRoutingData endpoint. Modify TrieRoutingData construction in MetadataStoreDirectory. Fix race conditions among writing operations in MetadataStoreDirectory.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387199996
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +116,11 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
- Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
- }
-
- return true;
- }, TestHelper.WAIT_DURATION), "Routing data path should be deleted after the tests.");
+ private void clearRoutingDataPath() {
+ for (String zkRealm : _baseAccessor
+ .getChildNames(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, AccessOption.PERSISTENT)) {
+ _baseAccessor.remove(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm,
+ AccessOption.PERSISTENT);
Review comment:
I believe we talked about this before implementing and decided to use `BaseDataAccessor`. This is good to know.
Regardless, how do you feel about using `HelixZkClient`? It's used in other tests as well (like the one for leader election), but it's a deprecated class. Is it really okay to use it among the tests? I'd expect the general consensus to be avoid using 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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386743572
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -155,6 +159,19 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
return routingData;
}
+ @Override
+ public boolean setNamespaceRoutingData(String namespace, Map<String, List<String>> routingData) {
+ synchronized (this) {
+ if (!_routingDataWriterMap.containsKey(namespace)) {
Review comment:
Sure.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386768268
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/MetadataStoreDirectoryAccessorTestBase.java
##########
@@ -100,23 +100,19 @@ public void beforeClass() throws Exception {
@AfterClass
public void afterClass() throws Exception {
System.clearProperty(MetadataStoreRoutingConstants.MSDS_SERVER_HOSTNAME_KEY);
+ _routingDataReader.close();
deleteRoutingDataPath();
}
- protected void deleteRoutingDataPath() throws Exception {
- Assert.assertTrue(TestHelper.verify(() -> {
- _zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
-
- for (String zk : _zkList) {
- if (ZK_SERVER_MAP.get(zk).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
+ protected void deleteRoutingDataPath() {
+ for (String zk : _zkList) {
+ 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);
Review comment:
We shouldn't remove the TestHelper block. Could we add that back? In general, please try to add a verify() block after cleanup.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387354610
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
##########
@@ -277,4 +315,27 @@ public void testChildChangeCallback() throws Exception {
return false;
}, TestHelper.WAIT_DURATION));
}
+
+ private void clearRoutingData() throws Exception {
+ Assert.assertTrue(TestHelper.verify(() -> {
+ for (String zk : _zkList) {
Review comment:
:+1:
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387317902
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
##########
@@ -277,4 +315,27 @@ public void testChildChangeCallback() throws Exception {
return false;
}, TestHelper.WAIT_DURATION));
}
+
+ private void clearRoutingData() throws Exception {
+ Assert.assertTrue(TestHelper.verify(() -> {
+ for (String zk : _zkList) {
Review comment:
`verify()` already retries, right?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387275928
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
##########
@@ -277,4 +315,27 @@ public void testChildChangeCallback() throws Exception {
return false;
}, TestHelper.WAIT_DURATION));
}
+
+ private void clearRoutingData() throws Exception {
+ Assert.assertTrue(TestHelper.verify(() -> {
+ for (String zk : _zkList) {
Review comment:
Could we add a retry logic on false?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386768495
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestMetadataStoreDirectoryAccessor.java
##########
@@ -452,6 +454,43 @@ public void testDeleteShardingKey() throws InvalidRoutingDataException {
Assert.assertEquals(getAllShardingKeysInTestRealm1(), expectedShardingKeysSet);
}
+ @Test(dependsOnMethods = "testDeleteShardingKey")
+ public void testSetRoutingData() throws InvalidRoutingDataException, IOException {
+ Map<String, List<String>> routingData = new HashMap<>();
+ routingData.put(TEST_REALM_1, TEST_SHARDING_KEYS_2);
+ routingData.put(TEST_REALM_2, TEST_SHARDING_KEYS_1);
+ String routingDataString = new ObjectMapper().writeValueAsString(routingData);
+
+ Map<String, String> badFormatRoutingData = new HashMap<>();
+ badFormatRoutingData.put(TEST_REALM_1, TEST_REALM_2);
+ badFormatRoutingData.put(TEST_REALM_2, TEST_REALM_1);
+ String badFormatRoutingDataString = new ObjectMapper().writeValueAsString(badFormatRoutingData);
Review comment:
Please create a constant OBJECT_MAPPER and use it throughout.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386743515
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -113,10 +113,14 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
_routingDataWriterMap.put(namespace, new ZkRoutingDataWriter(namespace, zkAddress));
// Populate realmToShardingKeys with ZkRoutingDataReader
- _realmToShardingKeysMap
- .put(namespace, _routingDataReaderMap.get(namespace).getRoutingData());
- _routingDataMap
- .put(namespace, new TrieRoutingData(_realmToShardingKeysMap.get(namespace)));
+ Map<String, List<String>> rawRoutingData =
+ _routingDataReaderMap.get(namespace).getRoutingData();
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
We are not throwing any exception because we allow this to happen. For logs, it's a good point - in my opinion, logging this may pollute the main log because in most cases this situation isn't even qualified as an error (when a namespace is initialized, this exception clause will happen because `rawRoutingData` is empty, which is a normal situation); however, when this is an actual error situation, it's better to log it. Since this logic is in data change callback, it's very easy to spam logs that are non-essential in this case, hence why I didn't log anything here.
Since I have conflicting views myself, I'm open to your suggestions.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387194671
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##########
@@ -220,6 +228,24 @@ public Response getRoutingData() {
return JSONRepresentation(responseMap);
}
+ @PUT
+ @Path("/routing-data")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response setRoutingData(String jsonContent) {
+ try {
+ Map<String, List<String>> routingData = new ObjectMapper()
Review comment:
Although I agree with the same change in `ZkRoutingDataWriter`, in this case it's actually unnecessary because `MetadataStoreDirectoryAccessor` has a per-request cycle. This object mapper will be reconstructed every time anyway. Given that, the object mapper is better a local variable because it's not used anywhere else.
----------------------------------------------------------------
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
[GitHub] [helix] narendly merged pull request #844: Implement setRoutingData
for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387191371
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -169,19 +186,31 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get all mapping under path: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get all mapping under path: Namespace " + namespace
+ + " contains invalid routing data!");
Review comment:
Agreed.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387232460
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestMetadataStoreDirectoryAccessor.java
##########
@@ -452,6 +454,43 @@ public void testDeleteShardingKey() throws InvalidRoutingDataException {
Assert.assertEquals(getAllShardingKeysInTestRealm1(), expectedShardingKeysSet);
}
+ @Test(dependsOnMethods = "testDeleteShardingKey")
+ public void testSetRoutingData() throws InvalidRoutingDataException, IOException {
+ Map<String, List<String>> routingData = new HashMap<>();
+ routingData.put(TEST_REALM_1, TEST_SHARDING_KEYS_2);
+ routingData.put(TEST_REALM_2, TEST_SHARDING_KEYS_1);
+ String routingDataString = new ObjectMapper().writeValueAsString(routingData);
+
+ Map<String, String> badFormatRoutingData = new HashMap<>();
+ badFormatRoutingData.put(TEST_REALM_1, TEST_REALM_2);
+ badFormatRoutingData.put(TEST_REALM_2, TEST_REALM_1);
+ String badFormatRoutingDataString = new ObjectMapper().writeValueAsString(badFormatRoutingData);
Review comment:
Reusing the inherited `OBJECT_MAPPER`. Applies to many other files as well.
----------------------------------------------------------------
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
[GitHub] [helix] NealSun96 commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387200248
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataWriter.java
##########
@@ -132,61 +132,86 @@ public void testSetRoutingData() {
public void testAddMetadataStoreRealmNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.addMetadataStoreRealm(DUMMY_REALM);
- Assert.assertEquals("PUT", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "PUT");
Review comment:
Sure.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387354656
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +123,14 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
+ private void clearRoutingDataPath() throws Exception {
Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
+ for (String zkRealm : _zkClient
+ .getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
+ _zkClient.delete(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm);
Review comment:
:+1:
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386767760
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataWriter.java
##########
@@ -132,61 +132,86 @@ public void testSetRoutingData() {
public void testAddMetadataStoreRealmNonLeader() {
MockWriter mockWriter = new MockWriter(DUMMY_NAMESPACE, ZK_ADDR);
mockWriter.addMetadataStoreRealm(DUMMY_REALM);
- Assert.assertEquals("PUT", mockWriter.calledRequest.getMethod());
+ Assert.assertEquals(mockWriter.calledRequest.getMethod(), "PUT");
Review comment:
Could we use a pre-defined constant instead of "PUT"? (Cue: HttpVerbs..) Same applies below.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387277011
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +123,14 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
+ private void clearRoutingDataPath() throws Exception {
Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
+ for (String zkRealm : _zkClient
+ .getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
+ _zkClient.delete(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm);
Review comment:
Need a retry while block
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386767589
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -130,17 +116,11 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
}
}
- private void deleteRoutingDataPath() throws Exception {
- Assert.assertTrue(TestHelper.verify(() -> {
- ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
-
- if (ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
- .exists(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- return false;
- }
-
- return true;
- }, TestHelper.WAIT_DURATION), "Routing data path should be deleted after the tests.");
+ private void clearRoutingDataPath() {
+ for (String zkRealm : _baseAccessor
+ .getChildNames(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, AccessOption.PERSISTENT)) {
+ _baseAccessor.remove(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + zkRealm,
+ AccessOption.PERSISTENT);
Review comment:
Nit: Outside of helix-core module, we should try to use ZkClient instead of BaseDataAccessor.
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386764843
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -113,10 +113,14 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
_routingDataWriterMap.put(namespace, new ZkRoutingDataWriter(namespace, zkAddress));
// Populate realmToShardingKeys with ZkRoutingDataReader
- _realmToShardingKeysMap
- .put(namespace, _routingDataReaderMap.get(namespace).getRoutingData());
- _routingDataMap
- .put(namespace, new TrieRoutingData(_realmToShardingKeysMap.get(namespace)));
+ Map<String, List<String>> rawRoutingData =
+ _routingDataReaderMap.get(namespace).getRoutingData();
+ _realmToShardingKeysMap.put(namespace, rawRoutingData);
+ try {
+ _routingDataMap.put(namespace, new TrieRoutingData(rawRoutingData));
+ } catch (InvalidRoutingDataException e) {
+ // Do not create TrieRoutingData if the routing data is invalid
Review comment:
Great that you've considered possible log pollution! I think a WARN log would be okay to add here. Note that we are not expecting very frequent writes :)
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r387275928
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/TestZkMetadataStoreDirectory.java
##########
@@ -277,4 +315,27 @@ public void testChildChangeCallback() throws Exception {
return false;
}, TestHelper.WAIT_DURATION));
}
+
+ private void clearRoutingData() throws Exception {
+ Assert.assertTrue(TestHelper.verify(() -> {
+ for (String zk : _zkList) {
Review comment:
Could we add the retry logic on false?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386766805
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataWriter.java
##########
@@ -332,8 +349,9 @@ private String constructUrlSuffix(String... urlParams) {
return String.join("", allUrlParameters);
}
- private boolean forwardRequestToLeader(String urlSuffix, HttpConstants.RestVerbs request_method,
- int expectedResponseCode) throws IllegalArgumentException {
+ private boolean buildAndSendRequestToLeader(String urlSuffix,
+ HttpConstants.RestVerbs request_method, int expectedResponseCode)
Review comment:
shouldWeUseCamelCaseFor request_method?
----------------------------------------------------------------
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
[GitHub] [helix] narendly commented on a change in pull request #844:
Implement setRoutingData for MetadataStoreDirectoryService
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #844: Implement setRoutingData for MetadataStoreDirectoryService
URL: https://github.com/apache/helix/pull/844#discussion_r386765344
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -169,19 +186,31 @@ private void init(String namespace, String zkAddress) throws InvalidRoutingDataE
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- if (!_routingDataMap.containsKey(namespace)) {
+ // Check _routingZkAddressMap first to see if namespace is included
+ if (!_routingZkAddressMap.containsKey(namespace)) {
throw new NoSuchElementException(
"Failed to get all mapping under path: Namespace " + namespace + " is not found!");
}
+ // If namespace is included but not routing data, it means the routing data is invalid
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalStateException("Failed to get all mapping under path: Namespace " + namespace
+ + " contains invalid routing data!");
Review comment:
Could we rephrase this to say, the routing data is either _empty_ or invalid?
----------------------------------------------------------------
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