You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/11/17 19:57:05 UTC
[GitHub] [helix] xyuanlu opened a new pull request #1540: Move subscribeForChange out of critical section
xyuanlu opened a new pull request #1540:
URL: https://github.com/apache/helix/pull/1540
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#1539
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
We found TaskScheduling stage is slow for task intensive workloads. From profiling result, we found the problem is:
TaskSchedulingStage.scheduleWorkflows -> getHelixPropertyStore() and CallBackHandler.invoke() all synchronized on zkHelixManager object, witch makes TaskSchedulingStage extremely slow.
This PR moves CallBackHandler.subscribeForChange(), witch takes majority of the time in invoke() out of the critical section.
### Tests
- [X] The following tests are written for this issue:
NA
- [X] The following is the result of the "mvn test" command on the appropriate module:
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestBatchMessage.testParticipantIncompatibleWithBatchMsg:355 expected:<true> but was:<false>
[INFO]
[ERROR] Tests run: 1250, Failures: 1, Errors: 0, Skipped: 0
rerun:
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.508 s - in org.apache.helix.integration.messaging.TestBatchMessage
```
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526500354
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges here might be overkill. Maybe it could be moved out later.
Review comment:
Why do we sync on this here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1540:
URL: https://github.com/apache/helix/pull/1540
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526532222
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,29 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges in critical section might be overkill.
+ // Maybe it could be moved out later.
+ synchronized (this) {
Review comment:
I think you already answered the question yourself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526505856
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges here might be overkill. Maybe it could be moved out later.
Review comment:
I guess it is possible for callBackHandler gets `Finalize` and a `callBack` at the same time. So the type checking and the subscribe should be in the same synced block..?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526520539
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges here might be overkill. Maybe it could be moved out later.
Review comment:
Please correct me if I am wrong. With batch enabled, it is possible that we get 'Finalize' and a 'callback' at the same time. If there is no lock in line 348 to 358, then it is possible that we do
1. type check first for 'callback' and passed,
2. type check for 'Finalize' and passed,
3. unsubscribe for 'Finalize'
4. Subscribe for 'callback'
There won't be such issue with the lock here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526506355
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ synchronized (this) {
if (!_expectTypes.contains(type)) {
logger.warn("Callback handler {} received event in wrong order. Listener: {}, path: {}, "
+ "expected types: {}, but was {}", _uid, _listener, _path, _expectTypes, type);
return;
-
}
_expectTypes = nextNotificationType.get(type);
if (type == Type.INIT || type == Type.FINALIZE || changeContext.getIsChildChange()) {
subscribeForChanges(changeContext.getType(), _path, _watchChild);
}
+ }
+
+ // This allows the Helix Manager to work with one change at a time
+ // TODO: Maybe we don't need to sync on _manager for all types of listener. PCould be a
+ // potential improvement candidate.
+ synchronized (_manager) {
Review comment:
Yes In my understanding, I think it is possible that we don't need to make all different onXXXChange to be sequential. So this `TODO` is added..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526500354
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges here might be overkill. Maybe it could be moved out later.
Review comment:
Why do we sync on this here? In fact, how do we reason it?
Assuming no batch, the calling path would be from enqueueTask(), which is called by handleXXX from ZkClient event thread. And also init() and reset()
Assuming batch, the calling path would be from _batchCallbackProcessor and also init() and reset().
init() is further called by either first time CallbackHandler construction or ZK session change.
It seems that ZK session change is the only places that there would be race condition, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1540:
URL: https://github.com/apache/helix/pull/1540#issuecomment-729305647
Should be good, but it would be great if you can rebase to the top of the master with more test fixes and see if it can pass without any error.
I can also help to trigger more github tests for you to ensure it is 100% safe.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r525619919
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
+ synchronized (this) {
if (!_expectTypes.contains(type)) {
logger.warn("Callback handler {} received event in wrong order. Listener: {}, path: {}, "
+ "expected types: {}, but was {}", _uid, _listener, _path, _expectTypes, type);
return;
-
}
_expectTypes = nextNotificationType.get(type);
if (type == Type.INIT || type == Type.FINALIZE || changeContext.getIsChildChange()) {
subscribeForChanges(changeContext.getType(), _path, _watchChild);
}
+ }
+
+ // This allows the listener to work with one change at a time
Review comment:
nit, "the listener" -> "the Helix Manager"?
Also, let's add a TODO here since it might be overkill as we discussed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526504248
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,29 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ // TODO: Having subscribeForChanges in critical section might be overkill.
+ // Maybe it could be moved out later.
+ synchronized (this) {
Review comment:
Why do we sync on this here? In fact, how do we reason it?
Assuming no batch, the calling path would be from enqueueTask(), which is called by handleXXX from ZkClient event thread. And also init() and reset()
Assuming batch, the calling path would be from _batchCallbackProcessor and also init() and reset().
init() is further called by either first time CallbackHandler construction or ZK session change.
It seems that ZK session change is the only places that there would be race condition, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526456079
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
+ synchronized (this) {
if (!_expectTypes.contains(type)) {
logger.warn("Callback handler {} received event in wrong order. Listener: {}, path: {}, "
+ "expected types: {}, but was {}", _uid, _listener, _path, _expectTypes, type);
return;
-
}
_expectTypes = nextNotificationType.get(type);
if (type == Type.INIT || type == Type.FINALIZE || changeContext.getIsChildChange()) {
subscribeForChanges(changeContext.getType(), _path, _watchChild);
}
+ }
+
+ // This allows the listener to work with one change at a time
Review comment:
TFTR. 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:
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] xyuanlu commented on pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on pull request #1540:
URL: https://github.com/apache/helix/pull/1540#issuecomment-730076839
Thanks for all the reviews.
This change is ready to be merged.
Final commit message:
Move subscribeForChange out of critical section to solve lock contention.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1540: Move subscribeForChange out of critical section
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1540:
URL: https://github.com/apache/helix/pull/1540#discussion_r526505380
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -340,26 +340,28 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
public void invoke(NotificationContext changeContext) throws Exception {
Type type = changeContext.getType();
long start = System.currentTimeMillis();
+ if (logger.isInfoEnabled()) {
+ logger.info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}",
+ Thread.currentThread().getId(), _uid, _path, _listener, type);
+ }
- // This allows the listener to work with one change at a time
- synchronized (_manager) {
- if (logger.isInfoEnabled()) {
- logger
- .info("{} START: CallbackHandler {}, INVOKE {} listener: {} type: {}", Thread.currentThread().getId(),
- _uid, _path, _listener, type);
- }
-
+ synchronized (this) {
if (!_expectTypes.contains(type)) {
logger.warn("Callback handler {} received event in wrong order. Listener: {}, path: {}, "
+ "expected types: {}, but was {}", _uid, _listener, _path, _expectTypes, type);
return;
-
}
_expectTypes = nextNotificationType.get(type);
if (type == Type.INIT || type == Type.FINALIZE || changeContext.getIsChildChange()) {
subscribeForChanges(changeContext.getType(), _path, _watchChild);
}
+ }
+
+ // This allows the Helix Manager to work with one change at a time
+ // TODO: Maybe we don't need to sync on _manager for all types of listener. PCould be a
+ // potential improvement candidate.
+ synchronized (_manager) {
Review comment:
why sync on _manager?
by the same argument as sync(this) above, it seems that there is no across _manager race condition, aside from ZK session change. Or am I missing something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org