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 2019/12/18 23:04:23 UTC
[GitHub] [helix] i3wangyi opened a new pull request #662: Reset all
listeners only in-memory operation
i3wangyi opened a new pull request #662: Reset all listeners only in-memory operation
URL: https://github.com/apache/helix/pull/662
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
#661
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
The code change will skip the getChildren() request on listener's removal, and the zkClient's unsubscribeAll will be used instead of unsubscribing a single path;
It also includes some log optimization
### Tests
TODO
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn 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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368314542
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -583,8 +580,7 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
* exists
*/
public void init() {
- logger.info("initializing CallbackHandler: " + this.toString() + " content: " + getContent());
-
+ logger.info("Initializing CallbackHandler, content: {}", getContent());
Review comment:
Why reduce the information here? The logger is constructed with CallbackHandler class, it is not clear for us which child class it might be if you remove the this.toString().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373750669
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
Added the test case of validating callback handlers before and after session expiry in `TestControllerLeaderhshipChange`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373748300
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
#681 is checked and it will be used to test details of this change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373747873
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
Good point! I will add the test (expire the session and compare the callhandlers before and after)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi closed pull request #662: Mitigate controller
leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi closed pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368302735
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -721,6 +716,21 @@ void reset(boolean isShutdown) {
}
}
+ /**
+ * The package private method only closes the batchCallbackProcessor thread
+ * *Caution*: currently it's only used during disconnecting from ZK and the
+ * listeners unsubscription will be taken care by zkClient directly
+ */
+ void closeCallbackProcessor() {
Review comment:
More precisely, it only closes batchProcessor. Rename the method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368303318
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
Have you double checked the listeners will be add back in handleNewSession?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375572904
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -721,6 +714,21 @@ void reset(boolean isShutdown) {
}
}
+ /**
+ * The package private method only closes the batchCallbackProcessor thread
+ * *Caution*: currently it's only used during disconnecting from ZK and the
+ * listeners unsubscription will be taken care by zkClient directly
+ */
+ void closeBatchCallbackProcessor() {
Review comment:
Besides my concern of not sending FINALIZE, this method is a subset of void reset(boolean isShutdown); I think you can avoid duplicate code.
Of course, please address my concern about not having the FINALIZE first. Then we can revisit this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368303318
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
Have you make sure the listeners will be add back once handleNewSession?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373748300
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
-
- if (_leaderElectionHandler != null) {
- _leaderElectionHandler.reset(true);
+ // remove every subscribed listener
+ if (_zkclient != null) {
+ _zkclient.unsubscribeAll();
Review comment:
#681 is checked in and it will be used to test details of this change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi edited a comment on issue #662: Mitigate
controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi edited a comment on issue #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#issuecomment-572805514
> Logically, it sounds good to me. But I think this is not the majority contribution in LeaderSwitch when controller is trying acquiring leadership, right?
>
> How much it reduces the overhead?
No, the optimization is only for leader -> standby, one data point shows a 10X latency improvement (from 1700ms -> 170ms); For optimizing the other part: standby -> leader, there're different options, I will explore them later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375667338
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
Review comment:
Sure, feel free to call for a short meeting with more people. My point is that we shall never trade-off correctness or functionality for performance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375604765
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
Review comment:
@jiajunwang I understand your concern. With the current helix structure & library dependency, many times there's really no easy way to satisfy everything while still keeping the code clean & short IMO, even we overall agree with the overall design here. Let's take it offline.
I will try to resolve rest concerns as much as I could and propose a new draft code structure
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375570851
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
Review comment:
The thing is that our customers also depend on the FINALIZE event to do the cleanup. I believe with this change disconnect will clean up all the listeners. But you break the agreement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373747823
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
Review comment:
Yeah. That's the design of using `unsubscribeAll`. Because the current callback handler's FINALIZE method is pretty expensive (involving read all children node under currentstates then remove them one by one).
I could perform tests that prove without the FINALIZE event, HelixManager could clean up all information when `disconnect()`. And the single callback handler could still use the FINALIZE event to un-register itself.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375571641
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -501,23 +499,20 @@ private void subscribeForChangesAsyn(NotificationContext.Type callbackType, Stri
private void subscribeForChanges(NotificationContext.Type callbackType, String path,
boolean watchChild) {
- logger.info("Subscribing changes listener to path: " + path + ", type: " + callbackType
- + ", listener: " + _listener);
+ logger.info(
+ "START:INVOKE subscribing changes listener on path: {}, callbackType: {}, listener: {}, isWatchChild: {}",
Review comment:
START:INVOKE/END:INVOKE are for the callbacks. Please don't use it here. It will be confusing to whoever debug later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi edited a comment on issue #662: Mitigate
controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi edited a comment on issue #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#issuecomment-572805514
> Logically, it sounds good to me. But I think this is not the majority contribution in LeaderSwitch when controller is trying acquiring leadership, right?
>
> How much it reduces the overhead?
No, the optimization is only for leader -> standby, one data point shows a 10X latency improvement (from 1700ms -> 170ms); For optimizing the other part of the story: standby -> leader, there're different options, I will explore them later in different PRs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368314570
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -697,8 +693,7 @@ public void reset() {
}
void reset(boolean isShutdown) {
- logger.info("Resetting CallbackHandler: {}. Is resetting for shutdown: {}.", this.toString(),
- isShutdown);
+ logger.info("Reset CallbackHandler. isShutdown: {}. Content: {}", isShutdown, getContent());
Review comment:
Same here, callback type is necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373747575
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -697,8 +693,7 @@ public void reset() {
}
void reset(boolean isShutdown) {
- logger.info("Resetting CallbackHandler: {}. Is resetting for shutdown: {}.", this.toString(),
- isShutdown);
+ logger.info("Reset CallbackHandler. isShutdown: {}. Content: {}", isShutdown, getContent());
Review comment:
The callback type is not passed in from the caller in the beginning. Do you want to pass that information? And getContent gives you all the information
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on issue #662: Mitigate controller
leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
dasahcc commented on issue #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#issuecomment-572797351
Logically, it sounds good to me. But I think this is not the majority contribution in LeaderSwitch when controller is trying acquiring leadership, right?
How much it reduces the overhead?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368315724
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -780,13 +780,10 @@ public void disconnect() {
// TODO reset user defined handlers only
// TODO Fix the issue that when connection disconnected, reset handlers will be blocked. -- JJ
- // This is because reset logic contains ZK operations.
- resetHandlers(true);
Review comment:
So we are not sending FINALIZE events on the reset anymore? I think this is problematic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373746832
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -501,23 +501,20 @@ private void subscribeForChangesAsyn(NotificationContext.Type callbackType, Stri
private void subscribeForChanges(NotificationContext.Type callbackType, String path,
boolean watchChild) {
- logger.info("Subscribing changes listener to path: " + path + ", type: " + callbackType
- + ", listener: " + _listener);
+ logger
+ .info("Subscribing changes listener on path: {}, callbackType: {}, listener: {}, isWatchChild: {}",
+ path, callbackType, _listener, watchChild);
long start = System.currentTimeMillis();
if (_eventTypes.contains(EventType.NodeDataChanged)
|| _eventTypes.contains(EventType.NodeCreated)
|| _eventTypes.contains(EventType.NodeDeleted)) {
- logger.info("Subscribing data change listener to path: " + path);
Review comment:
These logs are actually redundant. The private subscribeForChanges() will eventually call other private methods
```
private void subscribeChildChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
|| callbackType == NotificationContext.Type.CALLBACK) {
if (logger.isDebugEnabled()) {
logger.debug(_manager.getInstanceName() + " subscribes child-change. path: " + path
+ ", listener: " + _listener);
}
_zkClient.subscribeChildChanges(path, this);
} else if (callbackType == NotificationContext.Type.FINALIZE) {
logger.info(_manager.getInstanceName() + " unsubscribe child-change. path: " + path
+ ", listener: " + _listener);
_zkClient.unsubscribeChildChanges(path, this);
}
}
private void subscribeDataChange(String path, NotificationContext.Type callbackType) {
if (callbackType == NotificationContext.Type.INIT
|| callbackType == NotificationContext.Type.CALLBACK) {
if (logger.isDebugEnabled()) {
logger.debug(_manager.getInstanceName() + " subscribe data-change. path: " + path
+ ", listener: " + _listener);
}
_zkClient.subscribeDataChanges(path, this);
} else if (callbackType == NotificationContext.Type.FINALIZE) {
logger.info(_manager.getInstanceName() + " unsubscribe data-change. path: " + path
+ ", listener: " + _listener);
_zkClient.unsubscribeDataChanges(path, this);
}
}
```
To perform the actual listener subscription. I think keeping the verbose log in the last step is sufficient enough and you know helix.log is flooded with all listeners logs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373747402
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -583,8 +580,7 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
* exists
*/
public void init() {
- logger.info("initializing CallbackHandler: " + this.toString() + " content: " + getContent());
-
+ logger.info("Initializing CallbackHandler, content: {}", getContent());
Review comment:
The `getContent()` method has the richest information to tell everything
```
public String getContent() {
return "CallbackHandler{" + "_watchChild=" + _watchChild + ", _preFetchEnabled="
+ _preFetchEnabled + ", _batchModeEnabled=" + _batchModeEnabled + ", _path='" + _path + '\''
+ ", _listener=" + _listener + ", _changeType=" + _changeType + ", _manager=" + _manager
+ ", _zkClient=" + _zkClient + '}';
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r373747582
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -721,6 +716,21 @@ void reset(boolean isShutdown) {
}
}
+ /**
+ * The package private method only closes the batchCallbackProcessor thread
+ * *Caution*: currently it's only used during disconnecting from ZK and the
+ * listeners unsubscription will be taken care by zkClient directly
+ */
+ void closeCallbackProcessor() {
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r368314837
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -501,23 +501,20 @@ private void subscribeForChangesAsyn(NotificationContext.Type callbackType, Stri
private void subscribeForChanges(NotificationContext.Type callbackType, String path,
boolean watchChild) {
- logger.info("Subscribing changes listener to path: " + path + ", type: " + callbackType
- + ", listener: " + _listener);
+ logger
+ .info("Subscribing changes listener on path: {}, callbackType: {}, listener: {}, isWatchChild: {}",
+ path, callbackType, _listener, watchChild);
long start = System.currentTimeMillis();
if (_eventTypes.contains(EventType.NodeDataChanged)
|| _eventTypes.contains(EventType.NodeCreated)
|| _eventTypes.contains(EventType.NodeDeleted)) {
- logger.info("Subscribing data change listener to path: " + path);
Review comment:
Why removing these logs? While debugging, how do we know if this listener is configured with what kind of _eventTypes list? Since there are still some concerns or bug reports regarding callbacks, we'd better keep the detail logs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #662:
Mitigate controller leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#discussion_r375568553
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -697,8 +693,7 @@ public void reset() {
}
void reset(boolean isShutdown) {
- logger.info("Resetting CallbackHandler: {}. Is resetting for shutdown: {}.", this.toString(),
- isShutdown);
+ logger.info("Reset CallbackHandler. isShutdown: {}. Content: {}", isShutdown, getContent());
Review comment:
getContent should be good enough. Good point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] i3wangyi commented on issue #662: Mitigate controller
leadership switch latency - improve leader -> standby
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on issue #662: Mitigate controller leadership switch latency - improve leader -> standby
URL: https://github.com/apache/helix/pull/662#issuecomment-572805514
> Logically, it sounds good to me. But I think this is not the majority contribution in LeaderSwitch when controller is trying acquiring leadership, right?
>
> How much it reduces the overhead?
No, the optimization is for leader -> standby, one data point shows a 10X latency improvement (from 1700ms -> 170ms); For optimizing the other part: standby -> leader, there're different options, I will explore them later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org