You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/04/28 09:35:57 UTC
[GitHub] [helix] richardstartin opened a new pull request, #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
richardstartin opened a new pull request, #2072:
URL: https://github.com/apache/helix/pull/2072
…ZNRecord
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1970
### Description
In Apache Pinot, we first deserialize `ZNRecord`s and then construct `HelixProperty`s from them, and never use the `ZNRecord` again. `HelixProperty` makes a defensive copy of the `ZNRecord`, which is actually expensive enough to show up prominently in profiles of our helix threads. This change adds constructors to `HelixConstructor` which do not clone the `ZNRecord`. Backward compatibility is maintained, and all existing code will continue to clone the `ZNRecord`, but new code can be written to set the `clone` to false to avoid the expensive copy.
### Tests
No tests have been added as this change is trivial.
### Changes that Break Backward Compatibility (Optional)
Backward compatibility has been maintained. Existing code continues to clone the `ZNRecord`.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue merged pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
junkaixue merged PR #2072:
URL: https://github.com/apache/helix/pull/2072
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r860859346
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
Of course, this does not clone the `ZNRecord`, so code which assumes the `ZNRecord` has been mutated will not work properly, but these constructors are made available for advanced users of Helix, like Apache Pinot. In our code, the copy is needless and shows up as a surprisingly noticeable cost, which I would like to avoid incurring. This is why I took care to document the constructors; use at your own risk, take responsibility for it, but make it possible not to incur the cost.
Subclassing would not work because `HelixProperty` would need to be constructed to construct the subclass, which would result in a clone. It could work the other way around though, but this flag feels like a simple, easy to document, and backward compatible solution.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #2072:
URL: https://github.com/apache/helix/pull/2072#issuecomment-1112437858
@junkaixue I am happy for this to be merged when you are (but let's wait for a clean run of the tests after the variable rename).
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861163459
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,50 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
Review Comment:
@junkaixue default value is false here because there is no need to make a copy of an `ZNRecord` the caller hasn't passed 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
junkaixue commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861181859
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,50 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
Review Comment:
I see. That's a good point for this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861113704
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,49 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
}
/**
* Initialize the property with an existing ZNRecord
- * @param record
+ * @param record the record is cloned, updates to this record will not be reflected by the HelixProperty
*/
public HelixProperty(ZNRecord record) {
- this(record, record.getId());
+ this(record, true);
}
/**
- * Initialize the property with an existing ZNRecord with new record id
+ * Initialize the property with an existing ZNRecord
* @param record
+ * @param clone set to true to make a copy of the ZNRecord, false otherwise
+ */
+ public HelixProperty(ZNRecord record, boolean clone) {
Review Comment:
OK done, I went for `deepCopyRecord`
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r860842513
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
I took a cursory look and decided to add a thought here based on the context I have, so correct me if I'm wrong or missed any other details:
Because we "deep copy" `HelixProperty` and its internal `ZNRecord`, throughout Helix we can treat this as such, and that is, we can assume that it's immutable. But in the case that we do not clone it and simply take after the internal `ZNRecord` reference, such assumptions no longer hold and therefore may open up avenues where issues could creep in. And this particular change doesn't allow us to know whether its internal `ZNRecord` reference is being shared across different `HelixProperty` instances or not.
Could we think of alternative ways to achieve this if you think this is necessary? For example, add a subclass of `HelixProperty` that shallow copies `ZNRecord`, so that there is no ambiguity?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #2072:
URL: https://github.com/apache/helix/pull/2072#issuecomment-1112489283
@junkaixue the record is only copied if `deepCopyRecord` is true. The default value is true _except_ when the `ZNRecord` originates within a constructor.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r860842513
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
Correct me if I'm wrong, but because we "deep copy" `HelixProperty` and its internal `ZNRecord`, throughout Helix we can treat this as such, and that is, we can assume that it's immutable. But in the case that we do not clone it and simply take after the internal `ZNRecord` reference, such assumptions no longer hold and therefore may open up avenues where issues could creep in. And this particular change doesn't allow us to know whether its internal `ZNRecord` reference is being shared across different `HelixProperty` instances or not.
Could we think of alternative ways to achieve this if you think this is necessary? For example, add a subclass of `HelixProperty` that shallow copies `ZNRecord`, so that there is no ambiguity?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
junkaixue commented on PR #2072:
URL: https://github.com/apache/helix/pull/2072#issuecomment-1112481149
@richardstartin Thanks for making the change. But if we change it to deepCopyRecord, you may need to change the boolean value. Since the value "false" means not deep copying literally, the default constructor default value should be true.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on PR #2072:
URL: https://github.com/apache/helix/pull/2072#issuecomment-1112686075
@junkaixue the build has passed here, I'm happy for you to merge this whenever you are ready
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861163785
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,50 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
}
/**
* Initialize the property with an existing ZNRecord
- * @param record
+ * @param record a deep copy of the record is made, updates to this record will not be reflected
+ * by the HelixProperty
*/
public HelixProperty(ZNRecord record) {
- this(record, record.getId());
+ this(record, true);
Review Comment:
@junkaixue default value true here because it came from the caller.
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,50 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
}
/**
* Initialize the property with an existing ZNRecord
- * @param record
+ * @param record a deep copy of the record is made, updates to this record will not be reflected
+ * by the HelixProperty
*/
public HelixProperty(ZNRecord record) {
- this(record, record.getId());
+ this(record, true);
}
/**
- * Initialize the property with an existing ZNRecord with new record id
+ * Initialize the property with an existing ZNRecord
* @param record
+ * @param deepCopyRecord set to true to make a copy of the ZNRecord, false otherwise
+ */
+ public HelixProperty(ZNRecord record, boolean deepCopyRecord) {
+ this(record, record.getId(), deepCopyRecord);
+ }
+
+ /**
+ * Initialize the property with an existing ZNRecord with new record id
+ * @param record a deep copy of the record is made, updates to this record will not be reflected by the HelixProperty
* @param id
*/
public HelixProperty(ZNRecord record, String id) {
- _record = (record instanceof SessionAwareZNRecord) ? new SessionAwareZNRecord(record, id)
- : new ZNRecord(record, id);
+ this(record, id, true);
Review Comment:
@junkaixue default value true here because it came from the caller.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861196563
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
But you don't have that guarantee anyway - `HelixProperty` exposes a reference to its `_record` with `getRecord` and the `ZNRecord` itself is mutable.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861306409
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
In that case, I think this change is okay since we didn't have that guarantee to begin with. 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] richardstartin commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
richardstartin commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r860859346
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
Of course, this does not clone the `ZNRecord`, so code which assumes the `ZNRecord` has been copied will not work properly, but these constructors are made available for advanced users of Helix, like Apache Pinot. In our code, the copy is needless and shows up as a surprisingly noticeable cost, which I would like to avoid incurring. This is why I took care to document the constructors; use at your own risk, take responsibility for it, but make it possible not to incur the cost.
Subclassing would not work because `HelixProperty` would need to be constructed to construct the subclass, which would result in a clone. It could work the other way around though, but this flag feels like a simple, easy to document, and backward compatible solution.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861172247
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -39,7 +39,7 @@
* A wrapper class for ZNRecord. Used as a base class for IdealState, CurrentState, etc.
*/
public class HelixProperty {
Review Comment:
Yes, I understand the motivation behind this. That still doesn't give us a way to tell if the object is thoroughly immutable (because the underlying ZNRecord has been deep-copied) - I was thinking maybe adding a final `boolean` that can tell us whether it's mutable or immutable? Do you think that would be useful for the users of `HelixProperty`?
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on a diff in pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
junkaixue commented on code in PR #2072:
URL: https://github.com/apache/helix/pull/2072#discussion_r861108847
##########
helix-core/src/main/java/org/apache/helix/HelixProperty.java:
##########
@@ -156,25 +156,49 @@ public String toString() {
* @param id
*/
public HelixProperty(String id) {
- this(new ZNRecord(id), id);
+ this(new ZNRecord(id), id, false);
}
/**
* Initialize the property with an existing ZNRecord
- * @param record
+ * @param record the record is cloned, updates to this record will not be reflected by the HelixProperty
*/
public HelixProperty(ZNRecord record) {
- this(record, record.getId());
+ this(record, true);
}
/**
- * Initialize the property with an existing ZNRecord with new record id
+ * Initialize the property with an existing ZNRecord
* @param record
+ * @param clone set to true to make a copy of the ZNRecord, false otherwise
+ */
+ public HelixProperty(ZNRecord record, boolean clone) {
Review Comment:
NIT: Let's have a more clear naming for this. "clone" is a little bit confusing. Could be enableDeepCopy or something else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on pull request #2072: Issue #1970: allow clients to prevent HelixProperty from cloning the ZNRecord
Posted by GitBox <gi...@apache.org>.
junkaixue commented on PR #2072:
URL: https://github.com/apache/helix/pull/2072#issuecomment-1112511935
Since this is very fundament change, @richardstartin are you able to provide the test results? Github machine usually is slow and could cause unstable failure. Better to run it on local linux machine.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org