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/02/11 19:09:28 UTC
[GitHub] [helix] NealSun96 opened a new pull request #751: Implement
getAllMappingUnderPath and getMetadataStoreRealm
NealSun96 opened a new pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #750 0
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This PR implements `getAllMappingUnderPath` and `getMetadataStoreRealm` in `ZkMetadataStoreDirectory`. Also, `ZkRoutingDataReader` is updated specifically in `getRoutingData`: not having realm addresses and not having sharding keys for some realm addresses no longer raise exceptions.
### Tests
- [x] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 127, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 19.928 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 127, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.894 s
[INFO] Finished at: 2020-02-11T11:09:09-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] NealSun96 commented on a change in pull request #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377990567
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -104,11 +104,10 @@ public void testGetRoutingDataMissingMSRDChildren() {
_baseAccessor.create(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, new ZNRecord("test"),
AccessOption.PERSISTENT);
try {
- _zkRoutingDataReader.getRoutingData();
- Assert.fail("Expecting InvalidRoutingDataException");
+ Map<String, List<String>> routingData = _zkRoutingDataReader.getRoutingData();
Review comment:
Yes. I realized that the constructor was not populating `_routingDataMap`. That has been fixed.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378490721
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -53,55 +53,45 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
throw new IllegalArgumentException("Zk address cannot be null or empty!");
}
_zkAddress = zkAddress;
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
+ _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
+ new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
Review comment:
Ditto above. I used helix-style.xml to format the entire file and this was changed. Not sure if it was formatted previously.
----------------------------------------------------------------
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 #751: Implement
getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378469319
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
Has this been formatted properly?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377844743
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
Nit: let's use `NoSuchElementException` to keep it consistent with other methods in MSD?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377872125
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
> throw new NoSuchElementException("Namespace " + namespace + " does not exist!");
?
For read methods, please use NoSuchElement where appropriate.
For write methods, use IllegalArgument?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377853468
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
+ }
+ return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
Sorry about that, will fix.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377853190
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
The other methods in MSD use `IllegalArgumentException` such as the exact same check in `addMetadataStoreRealm`
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377845123
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
+ }
+ return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
Should we fix this error message?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377892533
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
Review comment:
I see what you mean. Agreed, changing.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377990453
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/MetadataStoreRoutingDataReader.java
##########
@@ -35,8 +35,7 @@
* @return a mapping from "metadata store realm addresses" to lists of "metadata store sharding
* keys", where the sharding keys in a value list all route to the realm address in the
* key
- * @throws InvalidRoutingDataException - when the routing data is malformed in any way that
- * disallows a meaningful mapping to be returned
+ * @throws InvalidRoutingDataException - when the routing data is missing
Review comment:
Done.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377845516
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/MetadataStoreRoutingDataReader.java
##########
@@ -35,8 +35,7 @@
* @return a mapping from "metadata store realm addresses" to lists of "metadata store sharding
* keys", where the sharding keys in a value list all route to the realm address in the
* key
- * @throws InvalidRoutingDataException - when the routing data is malformed in any way that
- * disallows a meaningful mapping to be returned
+ * @throws InvalidRoutingDataException - when the routing data is missing
Review comment:
Could we be more specific about this? What does it actually mean to have the routing data "missing"? Which ZNodes do not exist? Which fields do not exist?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377857618
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
ZNRecord record =
_zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child);
List<String> shardingKeys =
record.getListField(MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY);
- if (shardingKeys == null || shardingKeys.isEmpty()) {
- throw new InvalidRoutingDataException(
- "Realm address ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child
- + " does not have a value for key "
- + MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY
- + ". Routing ZooKeeper address: " + _zkAddress);
+ if (shardingKeys != null && !shardingKeys.isEmpty()) {
+ routingData.put(child, shardingKeys);
Review comment:
My opinion is that if a realm doesn't have sharding keys, it shouldn't be a part of the routing data, because it isn't providing "routing". Although I can understand that whoever tries to get all routing data may get surprised if the realm disappears.
If we are adding empty lists, I would have to change the behavior here also: https://github.com/apache/helix/pull/731/files#diff-cd0252f86478a5ece76fb628cd7edbfcR170
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377844856
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
+ }
+ return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
Review comment:
> NoSuchElementException
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378531693
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -53,55 +53,45 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
throw new IllegalArgumentException("Zk address cannot be null or empty!");
}
_zkAddress = zkAddress;
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
+ _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
+ new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
_routingDataListener = routingDataListener;
if (_routingDataListener != null) {
// Subscribe child changes
_zkClient.subscribeChildChanges(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, this);
// Subscribe data changes
for (String child : _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- _zkClient
- .subscribeDataChanges(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child,
- this);
+ _zkClient.subscribeDataChanges(
+ MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child, this);
}
}
}
/**
* Returns (realm, list of ZK path sharding keys) mappings.
* @return Map <realm, list of ZK path sharding keys>
- * @throws InvalidRoutingDataException
+ * @throws InvalidRoutingDataException - when the node on
+ * MetadataStoreRoutingConstants.ROUTING_DATA_PATH is missing
*/
- public Map<String, List<String>> getRoutingData()
- throws InvalidRoutingDataException {
+ public Map<String, List<String>> getRoutingData() throws InvalidRoutingDataException {
Map<String, List<String>> routingData = new HashMap<>();
- List<String> children;
+ List<String> allRealmAddresses;
try {
- children = _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
+ allRealmAddresses = _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
} catch (ZkNoNodeException e) {
throw new InvalidRoutingDataException(
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
- for (String child : children) {
+ for (String realmAddress : allRealmAddresses) {
ZNRecord record =
- _zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child);
+ _zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + realmAddress);
Review comment:
This IMO is not significant enough to create a static method for. The logic is far too simple for a method to be created. If we have repeated occurrences of string-building such as `MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + param1 + "/" + param2 + "/" + param3` then I agree it's necessary.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378491802
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
I'm using helix-style.xml as suggested by this doc: https://cwiki.apache.org/confluence/display/HELIX/Coding+Style . The linked doc come from https://iwww.corp.linkedin.com/wiki/cf/pages/viewpage.action?pageId=209597012 . helix-style-intellij.xml was not mentioned anywhere as far as I know.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378469405
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -53,55 +53,45 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
throw new IllegalArgumentException("Zk address cannot be null or empty!");
}
_zkAddress = zkAddress;
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
+ _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
+ new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
Review comment:
Why does this keep changing?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377854671
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
Review comment:
Agreed. Changing 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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378522346
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
Turns out that the onboarding doc links to an incorrect coding style page. Correcting 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] narendly commented on a change in pull request #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377960269
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##########
@@ -104,11 +104,10 @@ public void testGetRoutingDataMissingMSRDChildren() {
_baseAccessor.create(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, new ZNRecord("test"),
AccessOption.PERSISTENT);
try {
- _zkRoutingDataReader.getRoutingData();
- Assert.fail("Expecting InvalidRoutingDataException");
+ Map<String, List<String>> routingData = _zkRoutingDataReader.getRoutingData();
Review comment:
Do we need to add tests for the newly supported methods?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378522433
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
Turns out that the onboarding doc links to an incorrect coding style page. Correcting 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 issue #751: Implement
getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on issue #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#issuecomment-585461887
This PR is ready to be merged, approved by @narendly
Final commit message:
## Implement getAllMappingUnderPath and getMetadataStoreRealm in ZkMetadataStoreDirectory ##
This PR implements getAllMappingUnderPath and getMetadataStoreRealm in ZkMetadataStoreDirectory. Also, getRoutingData in ZkRoutingDataReader is updated : not having realm addresses and not having sharding keys for some realm addresses no longer raise exceptions. We now allow realms to not have sharding keys and return them as a part of routing data.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377957591
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/MetadataStoreRoutingDataReader.java
##########
@@ -35,8 +35,7 @@
* @return a mapping from "metadata store realm addresses" to lists of "metadata store sharding
* keys", where the sharding keys in a value list all route to the realm address in the
* key
- * @throws InvalidRoutingDataException - when the routing data is malformed in any way that
- * disallows a meaningful mapping to be returned
+ * @throws InvalidRoutingDataException - when the routing data is missing
Review comment:
That's a good point. You could write that in "ZK"RoutingDataReader.
----------------------------------------------------------------
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 #751: Implement
getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378477655
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -53,55 +53,45 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
throw new IllegalArgumentException("Zk address cannot be null or empty!");
}
_zkAddress = zkAddress;
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
+ _zkClient = DedicatedZkClientFactory.getInstance().buildZkClient(
+ new HelixZkClient.ZkConnectionConfig(zkAddress),
+ new HelixZkClient.ZkClientConfig().setZkSerializer(new ZNRecordSerializer()));
_routingDataListener = routingDataListener;
if (_routingDataListener != null) {
// Subscribe child changes
_zkClient.subscribeChildChanges(MetadataStoreRoutingConstants.ROUTING_DATA_PATH, this);
// Subscribe data changes
for (String child : _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH)) {
- _zkClient
- .subscribeDataChanges(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child,
- this);
+ _zkClient.subscribeDataChanges(
+ MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child, this);
}
}
}
/**
* Returns (realm, list of ZK path sharding keys) mappings.
* @return Map <realm, list of ZK path sharding keys>
- * @throws InvalidRoutingDataException
+ * @throws InvalidRoutingDataException - when the node on
+ * MetadataStoreRoutingConstants.ROUTING_DATA_PATH is missing
*/
- public Map<String, List<String>> getRoutingData()
- throws InvalidRoutingDataException {
+ public Map<String, List<String>> getRoutingData() throws InvalidRoutingDataException {
Map<String, List<String>> routingData = new HashMap<>();
- List<String> children;
+ List<String> allRealmAddresses;
try {
- children = _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
+ allRealmAddresses = _zkClient.getChildren(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
} catch (ZkNoNodeException e) {
throw new InvalidRoutingDataException(
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
- for (String child : children) {
+ for (String realmAddress : allRealmAddresses) {
ZNRecord record =
- _zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child);
+ _zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + realmAddress);
Review comment:
It seems quite several places use this concatenation `MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + realmAddress`. I suggest creating a static util method to build the path, which may look cleaner.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377854593
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/MetadataStoreRoutingDataReader.java
##########
@@ -35,8 +35,7 @@
* @return a mapping from "metadata store realm addresses" to lists of "metadata store sharding
* keys", where the sharding keys in a value list all route to the realm address in the
* key
- * @throws InvalidRoutingDataException - when the routing data is malformed in any way that
- * disallows a meaningful mapping to be returned
+ * @throws InvalidRoutingDataException - when the routing data is missing
Review comment:
Zk vocabulary shouldn't be used here because this is a generic interface. I can rework the wording here, maybe just reverting to the original one because it's still 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] narendly commented on a change in pull request #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377846650
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
Review comment:
Nit: let's name our variables better.
for (String realmName : allRealmNames) {
might be a better pick?
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377938345
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
ZNRecord record =
_zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child);
List<String> shardingKeys =
record.getListField(MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY);
- if (shardingKeys == null || shardingKeys.isEmpty()) {
- throw new InvalidRoutingDataException(
- "Realm address ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child
- + " does not have a value for key "
- + MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY
- + ". Routing ZooKeeper address: " + _zkAddress);
+ if (shardingKeys != null && !shardingKeys.isEmpty()) {
+ routingData.put(child, shardingKeys);
Review comment:
On a second thought, `getAllMetadataStoreRealms` is dependent on this. The behavior has to be changed 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] NealSun96 commented on a change in pull request #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377854671
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377853401
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/ZkMetadataStoreDirectory.java
##########
@@ -119,14 +119,20 @@ public ZkMetadataStoreDirectory(Map<String, String> routingZkAddressMap)
@Override
public Map<String, String> getAllMappingUnderPath(String namespace, String path) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
+ "Failed to get all mapping under path: Namespace " + namespace + " is not found!");
+ }
+ return _routingDataMap.get(namespace).getAllMappingUnderPath(path);
}
@Override
public String getMetadataStoreRealm(String namespace, String shardingKey) {
- // TODO: get it from routingData
- throw new UnsupportedOperationException();
+ if (!_routingDataMap.containsKey(namespace)) {
+ throw new IllegalArgumentException(
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r377847093
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -85,23 +85,14 @@ public ZkRoutingDataReader(String namespace, String zkAddress,
"Routing data directory ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH
+ " does not exist. Routing ZooKeeper address: " + _zkAddress);
}
- if (children == null || children.isEmpty()) {
- throw new InvalidRoutingDataException(
- "There are no metadata store realms defined. Routing ZooKeeper address: " + _zkAddress);
- }
for (String child : children) {
ZNRecord record =
_zkClient.readData(MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child);
List<String> shardingKeys =
record.getListField(MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY);
- if (shardingKeys == null || shardingKeys.isEmpty()) {
- throw new InvalidRoutingDataException(
- "Realm address ZNode " + MetadataStoreRoutingConstants.ROUTING_DATA_PATH + "/" + child
- + " does not have a value for key "
- + MetadataStoreRoutingConstants.ZNRECORD_LIST_FIELD_KEY
- + ". Routing ZooKeeper address: " + _zkAddress);
+ if (shardingKeys != null && !shardingKeys.isEmpty()) {
+ routingData.put(child, shardingKeys);
Review comment:
If shardingKeys is empty, we should still return an empty list, correct? Otherwise the resulting map will be missing the entire realm, which would be incorrect.
----------------------------------------------------------------
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 #751:
Implement getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378490384
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
The previous formatting was not done by this PR. This is the result of using helix-style.xml to auto-format.
Note: the previous formatting violates the first element in formatting guideline: the length exceeds 100 characters. (https://cwiki.apache.org/confluence/display/HELIX/Coding+Style)
----------------------------------------------------------------
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 #751: Implement
getAllMappingUnderPath and getMetadataStoreRealm
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #751: Implement getAllMappingUnderPath and getMetadataStoreRealm
URL: https://github.com/apache/helix/pull/751#discussion_r378453471
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/accessor/ZkRoutingDataReader.java
##########
@@ -36,8 +36,8 @@
import org.apache.helix.zookeeper.zkclient.exception.ZkNoNodeException;
import org.apache.zookeeper.Watcher;
-
-public class ZkRoutingDataReader implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
+public class ZkRoutingDataReader
+ implements MetadataStoreRoutingDataReader, IZkDataListener, IZkChildListener, IZkStateListener {
Review comment:
Do you have latest helix-style-intellij.xml imported in your IDE? This format shouldn't be changed if you have the style import. It would be better to keep the original code that is not your change.
----------------------------------------------------------------
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