[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379284824
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
Review comment:
A new endpoint which is equivalent to GET /sharding-keys?realm={realm} is
added: GET /metadata-store-realms/{realm}/sharding-keys
In my opinion, these 2 filters are valid for this resource. If remove one
query param, this method would only have 1 query param `prefix` and 1 if-else:
all sharding keys or sharding keys with prefix.
But again, I would like to offer this query param option as they are valid,
and it is also implemented now.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379281125
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
Review comment:
I think the endpoint already tells the behavior. That's why we need to
follow the REST API design principal.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379283232
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
My explanation above. It is OK to have if-else because we accept query
params. A better way to process query param is like we construct a query for DB:
```
query += param1,
query += param2
```
But we don't have a query layer here so if-else acts as this query layer.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379283598
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
Review comment:
A new endpoint which is equivalent to `GET /sharding-keys?realm={realm}` is
added: `GET /metadata-store-realms/{realm}/sharding-keys`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379282521
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
`?sharding-key=` is a query param to filter the result. In this case, a
single object being returned is acceptable.
It is like
`/employees` returns all employees
`/employees?name=david` could returns a single employee (assume name is
unique).
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm opened a new issue #764: Add created resource to json response for MSD rest put requests
pkuwm opened a new issue #764: Add created resource to json response for MSD rest put requests URL: https://github.com/apache/helix/issues/764 Current response of MSD REST PUT response returns only status code. As a REST API design principal, a PUT response should include the created resource. To make the response more descriptive, we would like to add the created resource in the response for: - addMetadataStoreRealm() - addShardingKey() 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 opened a new pull request #763: SharedZKClient
kaisun2000 opened a new pull request #763: SharedZKClient
URL: https://github.com/apache/helix/pull/763
### Issues
- [ x ] My PR addresses the following Helix issues and references them in
the PR description:
Resolve #762
### Description
- [ ] Here are some details about my PR, including screenshots of any UI
changes:
This diff added the implementation of building realm aware shared ZKClient
implementation to ShareZkClientFactory.
The diff intends to achieve this with minimal change and easy to reason
correctness in terms of keeping the zk client session management, sync and
async semantic and auto-reconnection upon session expiration intact.
### Tests
- [ ] The following tests are written for this issue:
To be done.
- [x ] The following is the result of the "mvn test" command on the
appropriate module:
"mvn -pl zookeeper-api test" pass though it does not seem to invoke any real
test.
### Commits
- [ ] 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"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the
following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379228170
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
Review comment:
I have a feeling that this path resource is again doing too many things. Do
you think we should consider breaking this up into different path resources?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379218684
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
Review comment:
could we also document the difference in behavior depending on whether the
query param is given?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379218365
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
try {
- Collection realms =
_metadataStoreDirectory.getAllMetadataStoreRealms(_namespace);
-
- responseMap = new HashMap<>(1);
- responseMap.put(MetadataStoreRoutingConstants.METADATA_STORE_REALMS,
realms);
+ if (shardingKey == null) {
+Collection realms =
_metadataStoreDirectory.getAllMetadataStoreRealms(_namespace);
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_REALMS, realms);
+return JSONRepresentation(responseMap);
+ } else {
Review comment:
Is this else needed?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379223922
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
Since this path resource seems to be doing two things, should we rename this
to `getMetadataStoreRealm()`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379227909
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
Or better yet, should we consider breaking this up into two different path
resources to eliminate if-elses?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
narendly commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory URL: https://github.com/apache/helix/pull/761#discussion_r379218026 ## File path: helix-rest/src/main/java/org/apache/helix/rest/metadatastore/constant/MetadataStoreRoutingConstants.java ## @@ -28,14 +28,21 @@ // Leader election ZNode for ZkRoutingDataWriter public static final String LEADER_ELECTION_ZNODE = "/_ZK_ROUTING_DATA_WRITER_LEADER"; + // Field name in JSON REST response of getting all metadata store namespaces. + public static final String METADATA_STORE_NAMESPACES = "namespaces"; + // Field name in JSON REST response of getting metadata store realms in one namespace. public static final String METADATA_STORE_REALMS = "metadataStoreRealms"; // Field name in JSON REST response of getting sharding keys in one realm. - public static final String SINGLE_METADATA_STORE_REALM = "metadataStoreRealm"; + public static final String SINGLE_METADATA_STORE_REALM = "realm"; // Field name in JSON REST response of getting sharding keys. public static final String SHARDING_KEYS = "shardingKeys"; + // Field name in JSON REST response related to one single sharding key. + public static final String SINGLE_SHARDING_KEY = "shardingKey"; + // Field name in JSON REST response of getting sharding keys with prefix. + public static final String SHARDING_KEY_PREFIX = "prefix"; Review comment: `SHARDING_KEY_PREFIX`'s naming got me thinking for a little bit and I couldn't figure it out. Could we rename this? I think something like `SHARDING_KEY_PATH_PREFIX` would be more descriptive. To also help with understanding, could we please add where it's going to be used? (like getAllMappingUnderPath() for example). 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 opened a new issue #762: Add RealmAware SharedZkFactory and SharedZkClient (also dedicated counter part) implementation
kaisun2000 opened a new issue #762: Add RealmAware SharedZkFactory and SharedZkClient (also dedicated counter part) implementation URL: https://github.com/apache/helix/issues/762 Implement the functionality to create realm aware dedicated/shared ZKClient using SharedZKClientFactory and DedicatedZKClientFactory which already exists. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379219384
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
I've thought about this carefully. As it is necessary, I've defined a POJO
class `MetadataStoreShardingKey` to represent the object mapping. The response
will look like:
```
{
"metadataStoreRealms": ["realm-0", "realm-1"]
}
```
or single ream with sharding key:
```
{
"shardingKey": "/a/b/c",
"realm": "realm-0"
}
```
I definitely agree that if the response of object is complex (has more
fields), we would need POJO classes to represent the responses.
For the time being, since the most of the responses are quite simple: a
list, I don't think we want to make it complex . I've tried to make all json
responses to have key-value mappings.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379216863
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
Review comment:
I've tried to make it as simple as possible. We have 3 different JAVA
methods in MetadataDirectory which accept different params.
https://github.com/apache/helix/blob/551ed63f98cf75b9953615386a0a9e1a9a566e33/helix-rest/src/main/java/org/apache/helix/rest/metadatastore/MetadataStoreDirectory.java#L52-L70
So even we set default values to the query params, we still have to check
the values and call the according 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly merged pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly merged pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces URL: https://github.com/apache/helix/pull/745 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
dasahcc commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379208017
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
try {
- Collection realms =
_metadataStoreDirectory.getAllMetadataStoreRealms(_namespace);
-
- responseMap = new HashMap<>(1);
- responseMap.put(MetadataStoreRoutingConstants.METADATA_STORE_REALMS,
realms);
+ if (shardingKey == null) {
+Collection realms =
_metadataStoreDirectory.getAllMetadataStoreRealms(_namespace);
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_REALMS, realms);
+return JSONRepresentation(responseMap);
+ } else {
Review comment:
Remove else clause.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
dasahcc commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379208234
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -66,24 +67,44 @@ private void postConstruct() {
}
/**
- * Gets all metadata store realms in a namespace with the endpoint.
+ * Gets all existing namespaces in the routing metadata store at endpoint:
+ * "GET /metadata-store-namespaces"
+ *
+ * @return Json response of all namespaces.
+ */
+ @GET
+ @Path("/metadata-store-namespaces")
+ public Response getAllNamespaces() {
+Collection namespaces = _metadataStoreDirectory.getAllNamespaces();
+Map> responseMap =
+
ImmutableMap.of(MetadataStoreRoutingConstants.METADATA_STORE_NAMESPACES,
namespaces);
+
+return JSONRepresentation(responseMap);
+ }
+
+ /**
+ * Gets all metadata store realms in a namespace at path: "GET
/metadata-store-realms",
+ * or gets a metadata store realm with the sharding key at path:
+ * "GET /metadata-store-realms?sharding-key={sharding-key}"
*
* @return Json representation of all realms.
*/
@GET
@Path("/metadata-store-realms")
- public Response getAllMetadataStoreRealms() {
-Map> responseMap;
+ public Response getAllMetadataStoreRealms(@QueryParam("sharding-key") String
shardingKey) {
Review comment:
Shall we define different response object instead of using plain Response.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #761: Add REST read endpoints to helix-rest for metadata store directory
dasahcc commented on a change in pull request #761: Add REST read endpoints to
helix-rest for metadata store directory
URL: https://github.com/apache/helix/pull/761#discussion_r379208465
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/resources/metadatastore/MetadataStoreDirectoryAccessor.java
##
@@ -111,41 +132,56 @@ public Response
deleteMetadataStoreRealm(@PathParam("realm") String realm) {
}
/**
- * Gets sharding keys mapped at path "HTTP GET /sharding-keys" which returns
all sharding keys in
- * a namespace, or path "HTTP GET /sharding-keys?realm={realmName}" which
returns sharding keys in
- * a realm.
+ * Gets all sharding keys mapped at paths:
+ * - "HTTP GET /sharding-keys" which returns all sharding keys in a
namespace.
+ * - "HTTP GET /sharding-keys?realm={realm}" which returns sharding keys in
the realm.
+ * - "HTTP GET /sharding-keys?prefix={prefix}" which returns sharding keys
that have the prefix.
+ * -- JSON response example for this path:
+ * {
+ * "prefix": "/sharding/key",
+ * "shardingKeys": [{
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/f"
+ *}, {
+ * "realm": "testRealm2",
+ * "shardingKey": "/sharding/key/1/e"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/b"
+ * }, {
+ * "realm": "testRealm1",
+ * "shardingKey": "/sharding/key/1/a"
+ * }]
+ * }
+ *
+ * - "HTTP GET /sharding-keys?realm={realm}={prefix}" which returns
sharding keys in the
+ * realm and that have the prefix.
*
- * @param realm Query param in endpoint path
- * @return Json representation of a map: shardingKeys -> collection of
sharding keys.
+ * @param realm Query param in endpoint path: metadata store realm.
+ * @param prefix Query param in endpoint path: prefix substring of sharding
key.
+ * @return Json representation for the sharding keys.
*/
@GET
@Path("/sharding-keys")
- public Response getShardingKeys(@QueryParam("realm") String realm) {
-Map responseMap;
-Collection shardingKeys;
+ public Response getShardingKeys(@QueryParam("realm") String realm,
+ @QueryParam("prefix") String prefix) {
try {
- // If realm is not set in query param, the endpoint is: "/sharding-keys"
- // to get all sharding keys in a namespace.
- if (realm == null) {
-shardingKeys = _metadataStoreDirectory.getAllShardingKeys(_namespace);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS.
-responseMap = new HashMap<>(1);
+ if (realm == null && prefix == null) {
+// For endpoint: "/sharding-keys" to get all sharding keys in a
namespace.
+return getAllShardingKeys();
+ } else if (prefix == null) {
+// For endpoint: "/sharding-keys?realm={realm}"
+return getAllShardingKeysInRealm(realm);
+ } else if (realm == null) {
+// For endpoint: "/sharding-keys?prefix={prefix}"
+return getAllShardingKeysUnderPath(prefix);
} else {
-// For endpoint: "/sharding-keys?realm={realmName}"
-shardingKeys =
_metadataStoreDirectory.getAllShardingKeysInRealm(_namespace, realm);
-// To avoid allocating unnecessary resource, limit the map's capacity
only for
-// SHARDING_KEYS and SINGLE_METADATA_STORE_REALM.
-responseMap = new HashMap<>(2);
-
responseMap.put(MetadataStoreRoutingConstants.SINGLE_METADATA_STORE_REALM,
realm);
+// For endpoint: "/sharding-keys?realm={realm}={prefix}"
+return getRealmShardingKeysUnderPath(realm, prefix);
Review comment:
This logic is very complex. Can we leverage the default value to simplify
the logic?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm opened a new pull request #761: Add REST read endpoints to helix-rest for metadata store directory
pkuwm opened a new pull request #761: Add REST read endpoints to helix-rest for
metadata store directory
URL: https://github.com/apache/helix/pull/761
### Issues
- [x] My PR addresses the following Helix issues and references them in the
PR description:
Implements #752
### Description
- [x] Here are some details about my PR, including screenshots of any UI
changes:
We have metadata store directory service to help scale out zookeeper.
Metadata store directory service provides REST APIs to access.
This PR adds remaining MSDS read endpoints to Helix REST.
### Tests
- [x] The following tests are written for this issue:
- 5 unit tests in TestMetadataStoreDirectoryAccessor
- [x] The following is the result of the "mvn test" command on the
appropriate module:
```
[INFO] Tests run: 145, Failures: 0, Errors: 0, Skipped: 0, Time elapsed:
26.54 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 145, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] BUILD SUCCESS
[INFO]
[INFO] Total time: 01:01 min
[INFO] Finished at: 2020-02-13T17:10:02-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"
### Documentation
- [ ] In case of new functionality, my PR adds documentation in the
following wiki page:
(Link the GitHub wiki you added)
### 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly merged pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly merged pull request #757: Add write REST endpoints to helix rest for metadata store directory URL: https://github.com/apache/helix/pull/757 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379173108
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -167,9 +252,73 @@ public void testGetShardingKeysInRealm() throws
IOException {
Assert.assertEquals(queriedShardingKeys, expectedShardingKeys);
}
+ @Test
+ public void testAddShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+Assert.assertFalse(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+"Realm does not have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+put(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 + "/sharding-keys/" +
TEST_SHARDING_KEY,
+null, Entity.entity("", MediaType.APPLICATION_JSON_TYPE),
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+put(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" + TEST_REALM_1 +
"/sharding-keys/"
++ TEST_SHARDING_KEY, null, Entity.entity("",
MediaType.APPLICATION_JSON_TYPE),
+Response.Status.CREATED.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.add(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
+ @Test(dependsOnMethods = "testAddShardingKey")
+ public void testDeleteShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+//Assert.assertTrue(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+//"Realm should have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+delete(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 +
"/sharding-keys/" + TEST_SHARDING_KEY,
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+delete(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" +
TEST_REALM_1 + "/sharding-keys/"
++ TEST_SHARDING_KEY, Response.Status.OK.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.remove(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
@AfterClass
- public void afterClass() {
-_zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
-.deleteRecursive(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
+ public void afterClass() throws Exception {
+_metadataStoreDirectory.close();
+deleteRoutingDataPath();
+ }
+
+ private void deleteRoutingDataPath() throws Exception {
Review comment:
Should we do an Assert.assertTrue on the return value of verify()?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379173157
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##
@@ -130,4 +133,18 @@ public void testGetRoutingDataMSRDChildEmptyValue() {
+ ". Routing ZooKeeper address: " + ZK_ADDR));
}
}
+
+ private void deleteRoutingDataPath() throws Exception {
Review comment:
Should we do an Assert.assertTrue on the return value of verify()?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
pkuwm commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory URL: https://github.com/apache/helix/pull/757#discussion_r379150234 ## File path: helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java ## @@ -27,38 +27,60 @@ import java.util.List; import java.util.Map; import java.util.Set; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import org.apache.helix.ZNRecord; -import org.apache.helix.manager.zk.ZNRecordSerializer; +import org.apache.helix.rest.metadatastore.MetadataStoreDirectory; +import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory; import org.apache.helix.rest.metadatastore.constant.MetadataStoreRoutingConstants; +import org.apache.helix.rest.metadatastore.exceptions.InvalidRoutingDataException; import org.apache.helix.rest.server.AbstractTestClass; import org.apache.helix.rest.server.util.JerseyUriRequestBuilder; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; import org.testng.Assert; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import static org.apache.helix.rest.common.HelixRestNamespace.DEFAULT_NAMESPACE_NAME; + Review comment: I don't have strong preference. I will address this in next PR for read endpoints. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
pkuwm commented on a change in pull request #757: Add write REST endpoints to
helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379149852
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -167,9 +254,58 @@ public void testGetShardingKeysInRealm() throws
IOException {
Assert.assertEquals(queriedShardingKeys, expectedShardingKeys);
}
+ @Test
+ public void testAddShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+Assert.assertFalse(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+"Realm does not have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+put(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 + "/sharding-keys/" +
TEST_SHARDING_KEY,
+null, Entity.entity("", MediaType.APPLICATION_JSON_TYPE),
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+put(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" + TEST_REALM_1 +
"/sharding-keys/"
++ TEST_SHARDING_KEY, null, Entity.entity("",
MediaType.APPLICATION_JSON_TYPE),
+Response.Status.CREATED.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.add(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
+ @Test(dependsOnMethods = "testAddShardingKey")
+ public void testDeleteShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+//Assert.assertTrue(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+//"Realm should have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+delete(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 +
"/sharding-keys/" + TEST_SHARDING_KEY,
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+delete(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" +
TEST_REALM_1 + "/sharding-keys/"
++ TEST_SHARDING_KEY, Response.Status.OK.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.remove(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
@AfterClass
public void afterClass() {
+_metadataStoreDirectory.close();
_zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
-.deleteRecursive(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
Review comment:
`deleteRecursively` is a sync blocking call. Once `deleteRecursively`
completes, the path is deleted.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379166015
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
Review comment:
@NealSun96 Let's add this in the Javadoc :)
> Yes it is. This scenario is when `path` is a sharding key that could be
added to the trie. Consider a trie that only has "/a/b/c" in it:
>
> 1. `getLongestPrefixNodeAlongPath("/a/b/c/x")` is the first case
(typically used during realm lookups), which returns "/a/b/c";
> 2. `getLongestPrefixNodeAlongPath("/a/b")` is the second case (typically
used during key scanning), which returns "/a/b";
> 3. `getLongestPrefixNodeAlongPath("/a/b/d")` is the last case, which
returns "/a/b". Note that "/a/b/d" is a valid sharding key to add.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379164173
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -40,7 +41,24 @@
import org.apache.zookeeper.data.Stat;
+/**
+ * The Realm-aware ZkClient interface.
+ * NOTE: "Realm-aware" does not necessarily mean that the RealmAwareZkClient
instance will be connecting to multiple ZK realms.
+ * On single-realm mode, RealmAwareZkClient will reject requests going out to
other ZK realms than the one set at initialization.
+ * On multi-realm mode, RealmAwareZkClient will connect to multiple ZK realms
but will reject EPHEMERAL AccessMode operations.
+ */
public interface RealmAwareZkClient {
+
+ /**
+ * Specifies which mode to run this RealmAwareZkClient on.
+ *
+ * SINGLE_REALM: CRUD, change subscription, and EPHEMERAL CreateMode are
supported.
+ * MULTI_REALM: CRUD and change subscription are supported. Operations
involving EPHEMERAL CreateMode will throw an UnsupportedOperationException.
+ */
+ enum MODE {
+SINGLE_REALM, MULTI_REALM
Review comment:
updated!
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379143730
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379143320
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
Review comment:
Related to the previous conversation and will be updated along the previous
part.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379142990
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
+ * path (meaning the traversal ended before the last node of the path is
reached), the provided
+ * path is not associated with any sharding key and can be added as a
sharding key without
+ * creating ambiguity cases among sharding keys.
* @param path - the path where the search is conducted
- * @param findLeafAlongPath - whether the search is for a leaf node on the
path
- * @return the node pointed by the path or a leaf node along the path
- * @throws NoSuchElementException - when the path points to nothing or when
no leaf node is
- * found
+ * @return a TrieNode that represents the longest prefix of the path
*/
- private TrieNode findTrieNode(String path, boolean findLeafAlongPath)
- throws NoSuchElementException {
+ private TrieNode getLongestPrefixNodeAlongPath(String path) {
Review comment:
Thank you for the feedback. If you know the logic but still find it
confusing, then that definitely is an indication. I coded this so I can't
possibly have an objective view on it.
While I agree that the current naming isn't the best, I'm not very keen
about "closest node" either. Let me keep the discussion open so that there may
be some second opinions coming in. I'll make sure to update the name before the
PR is checked in. Thanks!
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379142874
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -250,21 +268,22 @@ default long waitForEstablishedSession(long timeout,
TimeUnit timeUnit) {
* This is for backward compatibility and to avoid breaking the original
implementation of
* {@link org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener}.
*/
- class I0ItecIZkStateListenerHelixImpl
- implements
org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener {
+ class I0ItecIZkStateListenerHelixImpl implements
org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener {
Review comment:
Removed
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379138961
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -306,48 +326,63 @@ public int hashCode() {
}
/**
- * Configuration for creating a new ZkConnection.
+ * ZkConnection-related configs for creating an instance of
RealmAwareZkClient.
*/
- class ZkConnectionConfig {
-// Connection configs
-private final String _zkServers;
-private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+ class RealmAwareZkConnectionConfig {
+/**
+ * mode: Which mode the RealmAwareZkClientConfig should be created in
+ */
+private final MODE _mode;
+/**
+ * zkRealmShardingKey: used to deduce which ZK realm this
RealmAwareZkClientConfig should connect to.
+ * NOTE: this field will be ignored if MODE is MULTI_REALM!
+ */
+private final String _zkRealmShardingKey;
+private int _sessionTimeout = DEFAULT_SESSION_TIMEOUT;
-public ZkConnectionConfig(String zkServers) {
- _zkServers = zkServers;
+public RealmAwareZkConnectionConfig(MODE mode, String zkRealmShardingKey) {
+ if (mode == null) {
+throw new ZkClientException("Mode cannot be null!");
+ }
+ _mode = mode;
+ _zkRealmShardingKey = zkRealmShardingKey;
Review comment:
That's exactly 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379135620
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -306,48 +326,63 @@ public int hashCode() {
}
/**
- * Configuration for creating a new ZkConnection.
+ * ZkConnection-related configs for creating an instance of
RealmAwareZkClient.
*/
- class ZkConnectionConfig {
-// Connection configs
-private final String _zkServers;
-private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+ class RealmAwareZkConnectionConfig {
+/**
+ * mode: Which mode the RealmAwareZkClientConfig should be created in
+ */
+private final MODE _mode;
+/**
+ * zkRealmShardingKey: used to deduce which ZK realm this
RealmAwareZkClientConfig should connect to.
+ * NOTE: this field will be ignored if MODE is MULTI_REALM!
+ */
+private final String _zkRealmShardingKey;
+private int _sessionTimeout = DEFAULT_SESSION_TIMEOUT;
-public ZkConnectionConfig(String zkServers) {
- _zkServers = zkServers;
+public RealmAwareZkConnectionConfig(MODE mode, String zkRealmShardingKey) {
+ if (mode == null) {
+throw new ZkClientException("Mode cannot be null!");
+ }
+ _mode = mode;
+ _zkRealmShardingKey = zkRealmShardingKey;
Review comment:
Seemed to me the connection is per zk realm. Then the routing logic should
be at Factory layer, 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379134999
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -250,21 +268,22 @@ default long waitForEstablishedSession(long timeout,
TimeUnit timeUnit) {
* This is for backward compatibility and to avoid breaking the original
implementation of
* {@link org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener}.
*/
- class I0ItecIZkStateListenerHelixImpl
- implements
org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener {
+ class I0ItecIZkStateListenerHelixImpl implements
org.apache.helix.zookeeper.zkclient.deprecated.IZkStateListener {
Review comment:
Shall we remove Helix word?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
dasahcc commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379069586
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -40,7 +41,24 @@
import org.apache.zookeeper.data.Stat;
+/**
+ * The Realm-aware ZkClient interface.
+ * NOTE: "Realm-aware" does not necessarily mean that the RealmAwareZkClient
instance will be connecting to multiple ZK realms.
+ * On single-realm mode, RealmAwareZkClient will reject requests going out to
other ZK realms than the one set at initialization.
+ * On multi-realm mode, RealmAwareZkClient will connect to multiple ZK realms
but will reject EPHEMERAL AccessMode operations.
+ */
public interface RealmAwareZkClient {
+
+ /**
+ * Specifies which mode to run this RealmAwareZkClient on.
+ *
+ * SINGLE_REALM: CRUD, change subscription, and EPHEMERAL CreateMode are
supported.
+ * MULTI_REALM: CRUD and change subscription are supported. Operations
involving EPHEMERAL CreateMode will throw an UnsupportedOperationException.
+ */
+ enum MODE {
+SINGLE_REALM, MULTI_REALM
Review comment:
Make the second one separate line.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379133927
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
+ * path (meaning the traversal ended before the last node of the path is
reached), the provided
+ * path is not associated with any sharding key and can be added as a
sharding key without
+ * creating ambiguity cases among sharding keys.
* @param path - the path where the search is conducted
- * @param findLeafAlongPath - whether the search is for a leaf node on the
path
- * @return the node pointed by the path or a leaf node along the path
- * @throws NoSuchElementException - when the path points to nothing or when
no leaf node is
- * found
+ * @return a TrieNode that represents the longest prefix of the path
*/
- private TrieNode findTrieNode(String path, boolean findLeafAlongPath)
- throws NoSuchElementException {
+ private TrieNode getLongestPrefixNodeAlongPath(String path) {
Review comment:
Longest prefix is still confusing. When I read the code, I had to think
about what you meant by "longest prefix". Because the word "prefix' could mean
a host of different things depending on the context.
I believe I'm pretty familiar with the logic and if I found it a little
confusing even with the understanding, wouldn't that be an indication that
perhaps you could find a better, more intuitive name? :)
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379132737
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
Review comment:
Explain what the longest prefix is: The longest prefix means the closest
existing terminal node (sharding key).
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379132420
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
Review comment:
last-seen existing node?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient
and RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379131214
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -306,48 +326,63 @@ public int hashCode() {
}
/**
- * Configuration for creating a new ZkConnection.
+ * ZkConnection-related configs for creating an instance of
RealmAwareZkClient.
*/
- class ZkConnectionConfig {
-// Connection configs
-private final String _zkServers;
-private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+ class RealmAwareZkConnectionConfig {
+/**
+ * mode: Which mode the RealmAwareZkClientConfig should be created in
+ */
+private final MODE _mode;
+/**
+ * zkRealmShardingKey: used to deduce which ZK realm this
RealmAwareZkClientConfig should connect to.
+ * NOTE: this field will be ignored if MODE is MULTI_REALM!
+ */
+private final String _zkRealmShardingKey;
+private int _sessionTimeout = DEFAULT_SESSION_TIMEOUT;
-public ZkConnectionConfig(String zkServers) {
- _zkServers = zkServers;
+public RealmAwareZkConnectionConfig(MODE mode, String zkRealmShardingKey) {
Review comment:
FederatedZkClient is always multi. The implementation of FederateZkClient
can assume this without you passing a mode. The problem is that if you pass a
non-multi, it need to handle it 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly opened a new issue #760: Add RealmAwareZkClient and RealmAwareZkClientFactory
narendly opened a new issue #760: Add RealmAwareZkClient and RealmAwareZkClientFactory URL: https://github.com/apache/helix/issues/760 RealmAwareZkClient and RealmAwareZkClientFactory are going to be the top-level interfaces for other realm-aware ZkClient APIs (think FederatedZkClient, DedicatedZkClient, etc.). RealmAwareZkClient will support all the existing interface methods that HelixZkClient supports. RealmAwareZkClientFactory will be the interface implemented by SharedZkClientFactory and DedicatedZkClientFactory. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
pkuwm commented on a change in pull request #757: Add write REST endpoints to
helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379125110
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##
@@ -42,12 +42,16 @@
@BeforeClass
public void beforeClass() {
+ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
Review comment:
This should be a must because I believe there would be a latency for write
-> read. I will add them once we have write operations ready and enable the
asserts
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r37912
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -306,48 +326,63 @@ public int hashCode() {
}
/**
- * Configuration for creating a new ZkConnection.
+ * ZkConnection-related configs for creating an instance of
RealmAwareZkClient.
*/
- class ZkConnectionConfig {
-// Connection configs
-private final String _zkServers;
-private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+ class RealmAwareZkConnectionConfig {
+/**
+ * mode: Which mode the RealmAwareZkClientConfig should be created in
+ */
+private final MODE _mode;
+/**
+ * zkRealmShardingKey: used to deduce which ZK realm this
RealmAwareZkClientConfig should connect to.
+ * NOTE: this field will be ignored if MODE is MULTI_REALM!
+ */
+private final String _zkRealmShardingKey;
+private int _sessionTimeout = DEFAULT_SESSION_TIMEOUT;
-public ZkConnectionConfig(String zkServers) {
- _zkServers = zkServers;
+public RealmAwareZkConnectionConfig(MODE mode, String zkRealmShardingKey) {
Review comment:
I am thinking that we would use this for FederatedZkClient as well since
FederatedZkClient is an implementation of RealmAwareZkClient. With that said,
this is necessary because the mode will have to be different.
So mode is not necessarily encoded in the type.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379123140
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -167,9 +254,58 @@ public void testGetShardingKeysInRealm() throws
IOException {
Assert.assertEquals(queriedShardingKeys, expectedShardingKeys);
}
+ @Test
+ public void testAddShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+Assert.assertFalse(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+"Realm does not have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+put(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 + "/sharding-keys/" +
TEST_SHARDING_KEY,
+null, Entity.entity("", MediaType.APPLICATION_JSON_TYPE),
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+put(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" + TEST_REALM_1 +
"/sharding-keys/"
++ TEST_SHARDING_KEY, null, Entity.entity("",
MediaType.APPLICATION_JSON_TYPE),
+Response.Status.CREATED.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.add(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
+ @Test(dependsOnMethods = "testAddShardingKey")
+ public void testDeleteShardingKey() {
+Set expectedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+
+//Assert.assertTrue(expectedShardingKeysSet.contains(TEST_SHARDING_KEY),
+//"Realm should have sharding key: " + TEST_SHARDING_KEY);
+
+// Request that gets not found response.
+delete(NON_EXISTING_NAMESPACE_URI_PREFIX + TEST_REALM_1 +
"/sharding-keys/" + TEST_SHARDING_KEY,
+Response.Status.NOT_FOUND.getStatusCode());
+
+// Successful request.
+delete(TEST_NAMESPACE_URI_PREFIX + "/metadata-store-realms/" +
TEST_REALM_1 + "/sharding-keys/"
++ TEST_SHARDING_KEY, Response.Status.OK.getStatusCode());
+
+Set updatedShardingKeysSet = new HashSet<>(
+_metadataStoreDirectory.getAllShardingKeysInRealm(TEST_NAMESPACE,
TEST_REALM_1));
+expectedShardingKeysSet.remove(TEST_SHARDING_KEY);
+
+//Assert.assertEquals(updatedShardingKeysSet, expectedShardingKeysSet);
+ }
+
@AfterClass
public void afterClass() {
+_metadataStoreDirectory.close();
_zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
-.deleteRecursive(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
Review comment:
Probably a good idea to add a TestHelper.verify().
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory URL: https://github.com/apache/helix/pull/757#discussion_r379122606 ## File path: helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java ## @@ -27,38 +27,60 @@ import java.util.List; import java.util.Map; import java.util.Set; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import org.apache.helix.ZNRecord; -import org.apache.helix.manager.zk.ZNRecordSerializer; +import org.apache.helix.rest.metadatastore.MetadataStoreDirectory; +import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory; import org.apache.helix.rest.metadatastore.constant.MetadataStoreRoutingConstants; +import org.apache.helix.rest.metadatastore.exceptions.InvalidRoutingDataException; import org.apache.helix.rest.server.AbstractTestClass; import org.apache.helix.rest.server.util.JerseyUriRequestBuilder; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; import org.testng.Assert; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import static org.apache.helix.rest.common.HelixRestNamespace.DEFAULT_NAMESPACE_NAME; + Review comment: Since this is a test class, I am actually fine leaving it in either way if you think this improves readability. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379121949
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java
##
@@ -27,38 +27,60 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import javax.ws.rs.client.Entity;
+import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import org.apache.helix.ZNRecord;
-import org.apache.helix.manager.zk.ZNRecordSerializer;
+import org.apache.helix.rest.metadatastore.MetadataStoreDirectory;
+import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory;
import
org.apache.helix.rest.metadatastore.constant.MetadataStoreRoutingConstants;
+import
org.apache.helix.rest.metadatastore.exceptions.InvalidRoutingDataException;
import org.apache.helix.rest.server.AbstractTestClass;
import org.apache.helix.rest.server.util.JerseyUriRequestBuilder;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
+import static
org.apache.helix.rest.common.HelixRestNamespace.DEFAULT_NAMESPACE_NAME;
+
public class TestMetadataStoreDirectoryAccessor extends AbstractTestClass {
/*
* The following are constants to be used for testing.
*/
+ private static final String TEST_NAMESPACE_URI_PREFIX = "/namespaces/" +
TEST_NAMESPACE;
+ private static final String NON_EXISTING_NAMESPACE_URI_PREFIX =
+ "/namespaces/not-existed-namespace/metadata-store-realms/";
private static final String TEST_REALM_1 = "testRealm1";
private static final List TEST_SHARDING_KEYS_1 =
Arrays.asList("/sharding/key/1/a", "/sharding/key/1/b",
"/sharding/key/1/c");
private static final String TEST_REALM_2 = "testRealm2";
private static final List TEST_SHARDING_KEYS_2 =
Arrays.asList("/sharding/key/1/d", "/sharding/key/1/e",
"/sharding/key/1/f");
+ private static final String TEST_REALM_3 = "testRealm3";
+ private static final String TEST_SHARDING_KEY = "/sharding/key/1/x";
// List of all ZK addresses, each of which corresponds to a
namespace/routing ZK
private List _zkList;
+ private MetadataStoreDirectory _metadataStoreDirectory;
@BeforeClass
- public void beforeClass() {
+ public void beforeClass() throws InvalidRoutingDataException {
_zkList = new ArrayList<>(ZK_SERVER_MAP.keySet());
+_zkList.forEach(zk -> ZK_SERVER_MAP.get(zk).getZkClient()
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH));
Review comment:
Could add TestHelper.verify for making the test stable.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory URL: https://github.com/apache/helix/pull/757#discussion_r379121605 ## File path: helix-rest/src/test/java/org/apache/helix/rest/server/resources/zookeeper/TestMetadataStoreDirectoryAccessor.java ## @@ -27,38 +27,60 @@ import java.util.List; import java.util.Map; import java.util.Set; +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import org.apache.helix.ZNRecord; -import org.apache.helix.manager.zk.ZNRecordSerializer; +import org.apache.helix.rest.metadatastore.MetadataStoreDirectory; +import org.apache.helix.rest.metadatastore.ZkMetadataStoreDirectory; import org.apache.helix.rest.metadatastore.constant.MetadataStoreRoutingConstants; +import org.apache.helix.rest.metadatastore.exceptions.InvalidRoutingDataException; import org.apache.helix.rest.server.AbstractTestClass; import org.apache.helix.rest.server.util.JerseyUriRequestBuilder; +import org.apache.helix.zookeeper.datamodel.ZNRecord; +import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer; import org.testng.Assert; import org.testng.annotations.AfterClass; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; +import static org.apache.helix.rest.common.HelixRestNamespace.DEFAULT_NAMESPACE_NAME; + Review comment: Nit: could we avoid static imports and use them inline? 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379121334
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##
@@ -42,12 +42,16 @@
@BeforeClass
public void beforeClass() {
+ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
_zkRoutingDataReader = new ZkRoutingDataReader(DUMMY_NAMESPACE, ZK_ADDR,
null);
}
@AfterClass
public void afterClass() {
_zkRoutingDataReader.close();
+ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
Review comment:
Nit: a good addition to this would be a TestHelper.verify() that checks that
the deletion has completed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #757: Add write REST endpoints to helix rest for metadata store directory
narendly commented on a change in pull request #757: Add write REST endpoints
to helix rest for metadata store directory
URL: https://github.com/apache/helix/pull/757#discussion_r379121271
##
File path:
helix-rest/src/test/java/org/apache/helix/rest/metadatastore/accessor/TestZkRoutingDataReader.java
##
@@ -42,12 +42,16 @@
@BeforeClass
public void beforeClass() {
+ZK_SERVER_MAP.get(ZK_ADDR).getZkClient()
+.deleteRecursively(MetadataStoreRoutingConstants.ROUTING_DATA_PATH);
Review comment:
Nit: a good addition to this would be a TestHelper.verify() that checks that
the deletion has completed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379109153
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
Review comment:
Yes it is. This scenario is when `path` is a sharding key that could be
added to the trie. Consider a trie that only has "/a/b/c" in it:
1. `getLongestPrefixNodeAlongPath("/a/b/c/x")` is the first case (typically
used during realm lookups), which returns "/a/b/c";
2. `getLongestPrefixNodeAlongPath("/a/b")` is the second case (typically
used during key scanning), which returns "/a/b";
3. `getLongestPrefixNodeAlongPath("/a/b/d")` is the last case, which returns
"/a/b". Note that "/a/b/d" is a valid sharding key to add.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379110626
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
+ * path (meaning the traversal ended before the last node of the path is
reached), the provided
+ * path is not associated with any sharding key and can be added as a
sharding key without
+ * creating ambiguity cases among sharding keys.
* @param path - the path where the search is conducted
- * @param findLeafAlongPath - whether the search is for a leaf node on the
path
- * @return the node pointed by the path or a leaf node along the path
- * @throws NoSuchElementException - when the path points to nothing or when
no leaf node is
- * found
+ * @return a TrieNode that represents the longest prefix of the path
*/
- private TrieNode findTrieNode(String path, boolean findLeafAlongPath)
- throws NoSuchElementException {
+ private TrieNode getLongestPrefixNodeAlongPath(String path) {
Review comment:
To add to my last comment, "longest prefix" is very fitting of an
explanation. Given "/a/b/c", this method will attempt to return a node that
represents "/a/b/c"; if that doesn't exist, it tries "/a/b"; if not, "/a"; if
everything else fails, return "/".
IMO it's easier to say "longest prefix" then explaining "node that is
closest to `path` and is a parent to `path`".
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379109153
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
Review comment:
Yes it is. This scenario is when `path` is a sharding key that could be
added to the trie. Consider a trie that only has "/a/b/c" in it:
1. `getLongestPrefixNodeAlongPath("/a/b/c/x")` is the first case (typically
used during realm lookups);
2. `getLongestPrefixNodeAlongPath("/a/b")` is the second case (typically
used during key scanning);
3. `getLongestPrefixNodeAlongPath("/a/b/d")` is the last case. Note that
"/a/b/d" is a valid sharding key to add.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379107376
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
Review comment:
Ditto my explanation 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #759: Add validation logic to MSD write operations
NealSun96 commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379107200
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
+ * path (meaning the traversal ended before the last node of the path is
reached), the provided
+ * path is not associated with any sharding key and can be added as a
sharding key without
+ * creating ambiguity cases among sharding keys.
* @param path - the path where the search is conducted
- * @param findLeafAlongPath - whether the search is for a leaf node on the
path
- * @return the node pointed by the path or a leaf node along the path
- * @throws NoSuchElementException - when the path points to nothing or when
no leaf node is
- * found
+ * @return a TrieNode that represents the longest prefix of the path
*/
- private TrieNode findTrieNode(String path, boolean findLeafAlongPath)
- throws NoSuchElementException {
+ private TrieNode getLongestPrefixNodeAlongPath(String path) {
Review comment:
Appreciate the feedback on naming - it's something that I wish to discuss.
My original naming is along the line of "findClosestNode", however I find
myself have to explain the definition of "closest". For example, given
"/a/b/c", why is "/a" the closer than "/a/x/c" (if "/a/b" doesn't exist)?
Also, saying "closest node" when `path` sometimes doesn't point to a node is
strange.
I found that "longest prefix" is the most accurate definition of what this
method is doing - it finds the longest prefix of `path` that exists in this
trie.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379101610
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
Review comment:
Oh, never mind. This is the case where it's "successful".
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379099491
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
Review comment:
Is this scenario possible (assuming that the existing routing data is valid)?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379098852
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
Review comment:
"longest prefix"? Could we just call it the closest existing sharding key
node?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #759: Add validation logic to MSD write operations
narendly commented on a change in pull request #759: Add validation logic to
MSD write operations
URL: https://github.com/apache/helix/pull/759#discussion_r379096350
##
File path:
helix-rest/src/main/java/org/apache/helix/rest/metadatastore/TrieRoutingData.java
##
@@ -89,49 +87,55 @@ public String getMetadataStoreRealm(String path)
+ DELIMITER + "\" character: " + path);
}
-TrieNode leafNode = findTrieNode(path, true);
-return leafNode.getRealmAddress();
+TrieNode node = getLongestPrefixNodeAlongPath(path);
+if (!node.isShardingKey()) {
+ throw new NoSuchElementException(
+ "No sharding key found within the provided path. Path: " + path);
+}
+return node.getRealmAddress();
+ }
+
+ public boolean isShardingKeyInsertionValid(String shardingKey) {
+if (shardingKey.isEmpty() || !shardingKey.substring(0,
1).equals(DELIMITER)) {
+ throw new IllegalArgumentException(
+ "Provided shardingKey is empty or does not have a leading \"" +
DELIMITER
+ + "\" character: " + shardingKey);
+}
+
+TrieNode node = getLongestPrefixNodeAlongPath(shardingKey);
+return !node.isShardingKey() && !node.getPath().equals(shardingKey);
}
/**
- * If findLeafAlongPath is false, then starting from the root node, find the
trie node that the
- * given path is pointing to and return it; raise NoSuchElementException if
the path does
- * not point to any node. If findLeafAlongPath is true, then starting from
the root node, find the
- * leaf node along the provided path; raise NoSuchElementException if the
path does not
- * point to any node or if there is no leaf node along the path.
+ * Given a path, find a trie node that represents the longest prefix of the
path. For example,
+ * given "/a/b/c", the method starts at "/", and attempts to reach "/a",
then attempts to reach
+ * "/a/b", then ends on "/a/b/c"; if any of the node doesn't exist, the
traversal terminates and
+ * the last existing node is returned.
+ * Note:
+ * 1. When the returned TrieNode is a sharding key, it is the only sharding
key along the
+ * provided path (the path points to this sharding key);
+ * 2. When the returned TrieNode is not a sharding key but it represents the
provided path, the
+ * provided path is a prefix(parent) to a sharding key;
+ * 3. When the returned TrieNode is not a sharding key and it does not
represent the provided
+ * path (meaning the traversal ended before the last node of the path is
reached), the provided
+ * path is not associated with any sharding key and can be added as a
sharding key without
+ * creating ambiguity cases among sharding keys.
* @param path - the path where the search is conducted
- * @param findLeafAlongPath - whether the search is for a leaf node on the
path
- * @return the node pointed by the path or a leaf node along the path
- * @throws NoSuchElementException - when the path points to nothing or when
no leaf node is
- * found
+ * @return a TrieNode that represents the longest prefix of the path
*/
- private TrieNode findTrieNode(String path, boolean findLeafAlongPath)
- throws NoSuchElementException {
+ private TrieNode getLongestPrefixNodeAlongPath(String path) {
Review comment:
The name for this method is a little confusing. Do you think something like
findClosestParentNode or findClosetShardingKeyNode or findClosestTerminalNode
would be easier to understand?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient
and RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379087412
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/RealmAwareZkClient.java
##
@@ -306,48 +326,63 @@ public int hashCode() {
}
/**
- * Configuration for creating a new ZkConnection.
+ * ZkConnection-related configs for creating an instance of
RealmAwareZkClient.
*/
- class ZkConnectionConfig {
-// Connection configs
-private final String _zkServers;
-private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+ class RealmAwareZkConnectionConfig {
+/**
+ * mode: Which mode the RealmAwareZkClientConfig should be created in
+ */
+private final MODE _mode;
+/**
+ * zkRealmShardingKey: used to deduce which ZK realm this
RealmAwareZkClientConfig should connect to.
+ * NOTE: this field will be ignored if MODE is MULTI_REALM!
+ */
+private final String _zkRealmShardingKey;
+private int _sessionTimeout = DEFAULT_SESSION_TIMEOUT;
-public ZkConnectionConfig(String zkServers) {
- _zkServers = zkServers;
+public RealmAwareZkConnectionConfig(MODE mode, String zkRealmShardingKey) {
Review comment:
If we think MODE is already encoded in the Type (class hierarchy), we don't
need this in the constructor. Based on looking at the psuedo code I drew, I
don't fee we really need to pass this in.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 commented on issue #759: Add validation logic to MSD write operations
NealSun96 commented on issue #759: Add validation logic to MSD write operations URL: https://github.com/apache/helix/pull/759#issuecomment-585933576 **To all:** As I attempt to format the relevant files, it turns out that the formatting on my IDE disagrees with some of the existing files (using the latest `helix-style-intellij.xml`). I'm not sure if it's due to the files not formatted correctly previously or misconfiguration on my end. To reduce noise in diff, formatting has been temporarily held off. Once the logic has been reviewed, I'll create a commit for formatting only, where the formatting can be reviewed. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #729: Implement Helix API for updating customized state
dasahcc commented on a change in pull request #729: Implement Helix API for
updating customized state
URL: https://github.com/apache/helix/pull/729#discussion_r379066046
##
File path:
helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProviderFactory.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.customizedstate;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashMap;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Singleton factory that build customized state provider.
+ */
+public class CustomizedStateProviderFactory {
+ private static Logger LOG =
LoggerFactory.getLogger(CustomizedStateProvider.class);
+ private final HashMap
_customizedStateProviderMap =
+ new HashMap<>();
+ private HelixManager _helixManager;
+
+ protected CustomizedStateProviderFactory() {
+ }
+
+ private static class SingletonHelper {
+private static final CustomizedStateProviderFactory INSTANCE =
+new CustomizedStateProviderFactory();
+ }
+
+ public static CustomizedStateProviderFactory getInstance() {
+return SingletonHelper.INSTANCE;
+ }
+
+ public CustomizedStateProvider buildCustomizedStateProvider(String
instanceName) {
+if (_helixManager == null) {
+ throw new HelixException("Helix Manager has not been set yet.");
+} else {
+ return buildCustomizedStateProvider(_helixManager, instanceName);
+}
Review comment:
Remove else clause.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #729: Implement Helix API for updating customized state
dasahcc commented on a change in pull request #729: Implement Helix API for
updating customized state
URL: https://github.com/apache/helix/pull/729#discussion_r379066208
##
File path:
helix-core/src/main/java/org/apache/helix/customizedstate/CustomizedStateProviderFactory.java
##
@@ -0,0 +1,81 @@
+package org.apache.helix.customizedstate;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.HashMap;
+import org.apache.helix.HelixException;
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Singleton factory that build customized state provider.
+ */
+public class CustomizedStateProviderFactory {
+ private static Logger LOG =
LoggerFactory.getLogger(CustomizedStateProvider.class);
+ private final HashMap
_customizedStateProviderMap =
+ new HashMap<>();
+ private HelixManager _helixManager;
+
+ protected CustomizedStateProviderFactory() {
+ }
+
+ private static class SingletonHelper {
+private static final CustomizedStateProviderFactory INSTANCE =
+new CustomizedStateProviderFactory();
+ }
+
+ public static CustomizedStateProviderFactory getInstance() {
+return SingletonHelper.INSTANCE;
+ }
+
+ public CustomizedStateProvider buildCustomizedStateProvider(String
instanceName) {
+if (_helixManager == null) {
+ throw new HelixException("Helix Manager has not been set yet.");
+} else {
+ return buildCustomizedStateProvider(_helixManager, instanceName);
+}
+ }
+
+ /**
+ * Build a customized state provider based on the specified input. If the
instance already has a
+ * provider, return it. Otherwise, build a new one and put it in the map.
+ * @param helixManager The helix manager that belongs to the instance
+ * @param instanceName The name of the instance
+ * @return CustomizedStateProvider
+ */
+ public CustomizedStateProvider buildCustomizedStateProvider(HelixManager
helixManager,
+ String instanceName) {
+synchronized (_customizedStateProviderMap) {
+ if (_customizedStateProviderMap.get(instanceName) != null) {
+return _customizedStateProviderMap.get(instanceName);
+ } else {
Review comment:
Same here. Remove else clause.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379065539
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/factory/RealmAwareZkClientFactory.java
##
@@ -19,5 +19,13 @@
* under the License.
*/
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+
+
+/**
+ * Creates an instance of RealmAwareZkClient.
+ */
public interface RealmAwareZkClientFactory {
+ RealmAwareZkClient
buildZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
Review comment:
Sure, that's a good idea. Let me add it 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
narendly commented on a change in pull request #745: Add RealmAwareZkClient and
RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379065061
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/HelixZkClient.java
##
@@ -19,8 +19,178 @@
* under the License.
*/
+import org.apache.helix.zookeeper.zkclient.serialize.BasicZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.SerializableSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+
+
/**
+ * Deprecated - please use RealmAwareZkClient instead.
+ *
* HelixZkClient interface that follows the supported API structure of
RealmAwareZkClient.
*/
+@Deprecated
public interface HelixZkClient extends RealmAwareZkClient {
+
+ /**
+ * Deprecated - please use RealmAwareZkClient and
RealmAwareZkConnectionConfig instead.
+ *
+ * Configuration for creating a new ZkConnection.
+ */
+ @Deprecated
+ class ZkConnectionConfig {
+// Connection configs
+private final String _zkServers;
+private int _sessionTimeout = HelixZkClient.DEFAULT_SESSION_TIMEOUT;
+
+public ZkConnectionConfig(String zkServers) {
+ _zkServers = zkServers;
+}
+
+@Override
+public boolean equals(Object obj) {
+ if (obj == this) {
+return true;
+ }
+ if (!(obj instanceof HelixZkClient.ZkConnectionConfig)) {
+return false;
+ }
+ HelixZkClient.ZkConnectionConfig configObj =
(HelixZkClient.ZkConnectionConfig) obj;
+ return (_zkServers == null && configObj._zkServers == null || _zkServers
!= null && _zkServers
+ .equals(configObj._zkServers)) && _sessionTimeout ==
configObj._sessionTimeout;
+}
+
+@Override
+public int hashCode() {
+ return _sessionTimeout * 31 + _zkServers.hashCode();
+}
+
+@Override
+public String toString() {
+ return (_zkServers + "_" + _sessionTimeout).replaceAll("[\\W]", "_");
+}
+
+public HelixZkClient.ZkConnectionConfig setSessionTimeout(Integer
sessionTimeout) {
+ this._sessionTimeout = sessionTimeout;
+ return this;
+}
+
+public String getZkServers() {
+ return _zkServers;
+}
+
+public int getSessionTimeout() {
+ return _sessionTimeout;
+}
+ }
+
+ /**
+ * Deprecated - please use RealmAwareZkClient and RealmAwareZkClientConfig
instead.
+ *
+ * Configuration for creating a new HelixZkClient with serializer and
monitor.
+ */
+ @Deprecated
+ class ZkClientConfig {
Review comment:
@kaisun2000
I tried that but that is going to break backward-compatibility because of
different return types in the setter methods.
To illustrate:
`public ZkClientConfig setZkSerializer(PathBasedZkSerializer zkSerializer)`
is what we have in HelixZkClient. But if this class extended
RealmAwareZkClientConfig, then it will be returning an instance of
RealmAwareZkClientConfig, which would break the existing methods in Shared and
DedicatedZkClientFactories.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient and RealmAwareZkClientFactory interfaces
kaisun2000 commented on a change in pull request #745: Add RealmAwareZkClient
and RealmAwareZkClientFactory interfaces
URL: https://github.com/apache/helix/pull/745#discussion_r379057281
##
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/factory/RealmAwareZkClientFactory.java
##
@@ -19,5 +19,13 @@
* under the License.
*/
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+
+
+/**
+ * Creates an instance of RealmAwareZkClient.
+ */
public interface RealmAwareZkClientFactory {
+ RealmAwareZkClient
buildZkClient(RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig,
Review comment:
To achieve feature parity of HelixZkClientInterface, shall we add this
constructor too?
` public RealmAwareZkClient buildZkClient(
RealmAwareZkClient.RealmAwareZkConnectionConfig connectionConfig) {
return buildZkClient(connectConfig, new
RealmAwareZkClient.RealmAwareZkClientConfig());
}`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
[GitHub] [helix] NealSun96 opened a new issue #758: Add validation logic to MetadataStoreDirectory write methods
NealSun96 opened a new issue #758: Add validation logic to MetadataStoreDirectory write methods URL: https://github.com/apache/helix/issues/758 The metadata storage routing data stored in Zookeeper is the source of truth for all related services, and therefore, it needs to stay well maintained and should always be valid. As a result, validation logic is implemented on all writing operations towards the routing data - when a user attempts to add a sharding key to some existing routing data, validation logic is used to confirm that the addition will leave the resulting routing data in a valid state. 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
