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/09/09 01:11:31 UTC
[GitHub] [helix] kaisun2000 opened a new pull request #1355: Enhance logging for CallbackHandler and Zkclient
kaisun2000 opened a new pull request #1355:
URL: https://github.com/apache/helix/pull/1355
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
resolve #1352
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
CallbackHandlder and ZkClient methods are invoked over multiple threads.
It can be hard to correlate one object's execution threads over multiple
java threads. This poses a serious issue in production log examination.
We propose to add unique id to each methods' logging to help the
correlation and also track the object's life cycle.
### Tests
- [x] The following tests are written for this issue:
None
- [x] The following is the result of the "mvn test" command on the appropriate module:
running
### 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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r492467279
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -660,24 +666,31 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
//TODO: avoid calling getChildren for path that does not exist
if (_changeType == CUSTOMIZED_STATE_ROOT) {
logger.warn(
- "Failed to subscribe child/data change on path: {}, listener: {}. Instance "
- + "does not support Customized State!", path, _listener);
+ "CallbackHandler {}, Failed to subscribe child/data change on path: {}, listener: {}. Instance "
+ + "does not support Customized State!", this.toString(), path, _listener);
} else {
- logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
- _listener, e);
+ logger.warn("CallbackHandler {}, Failed to subscribe child/data change. path: {}, listener: {}",
+ this.toString(), path, _listener, e);
}
}
}
}
long end = System.currentTimeMillis();
- logger.info("Subscribing to path: {} took: {}", path, (end - start));
+ logger.info("CallbackHandler{}, Subscribing to path: {} took: {}", this.toString(), path, (end - start));
}
public EventType[] getEventTypes() {
return (EventType[]) _eventTypes.toArray();
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
+ return stringBuilder.toString();
Review comment:
changed .
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493788291
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -105,7 +105,9 @@
@PreFetchChangedData(enabled = false)
public class CallbackHandler implements IZkChildListener, IZkDataListener {
private static Logger logger = LoggerFactory.getLogger(CallbackHandler.class);
+ private static final AtomicLong CALLBACK_HANDLER_UI = new AtomicLong();
Review comment:
@kaisun2000 This one? `CALLBACK_HANDLER_UI` -> `CALLBACK_HANDLER_UID`
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r492467105
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -220,7 +227,7 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
_asyncCallRetryThread = new ZkAsyncRetryThread(zkConnection.getServers());
_asyncCallRetryThread.start();
- LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", _uid, _asyncCallRetryThread.getId());
+ LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", this.toString(), _asyncCallRetryThread.getId());
Review comment:
changed,
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
Review comment:
changed to final
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
Review comment:
changed.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -660,24 +666,31 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
//TODO: avoid calling getChildren for path that does not exist
if (_changeType == CUSTOMIZED_STATE_ROOT) {
logger.warn(
- "Failed to subscribe child/data change on path: {}, listener: {}. Instance "
- + "does not support Customized State!", path, _listener);
+ "CallbackHandler {}, Failed to subscribe child/data change on path: {}, listener: {}. Instance "
+ + "does not support Customized State!", this.toString(), path, _listener);
} else {
- logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
- _listener, e);
+ logger.warn("CallbackHandler {}, Failed to subscribe child/data change. path: {}, listener: {}",
+ this.toString(), path, _listener, e);
}
}
}
}
long end = System.currentTimeMillis();
- logger.info("Subscribing to path: {} took: {}", path, (end - start));
+ logger.info("CallbackHandler{}, Subscribing to path: {} took: {}", this.toString(), path, (end - start));
}
public EventType[] getEventTypes() {
return (EventType[]) _eventTypes.toArray();
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
+ return stringBuilder.toString();
Review comment:
changed .
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493866569
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -105,7 +105,9 @@
@PreFetchChangedData(enabled = false)
public class CallbackHandler implements IZkChildListener, IZkDataListener {
private static Logger logger = LoggerFactory.getLogger(CallbackHandler.class);
+ private static final AtomicLong CALLBACK_HANDLER_UI = new AtomicLong();
Review comment:
good catch. fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493901815
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -354,10 +359,10 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
// async mode only applicable to CALLBACK from ZK, During INIT and FINALIZE invoke the
// callback's immediately.
if (_batchModeEnabled && changeContext.getType() == NotificationContext.Type.CALLBACK) {
- logger.debug("Enqueuing callback");
+ logger.debug("Callbackhandler {}, Enqueuing callback", this._uid );
if (!isReady()) {
- logger.info("CallbackHandler is not ready, ignore change callback from path: {}, for "
- + "listener: {}", _path, _listener);
+ logger.info("CallbackHandler {} is not ready, ignore change callback from path: {}, for "
+ + "listener: {}", this._uid, _path, _listener);
Review comment:
nit, "this." in "this._uid" is not necessary, 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] kaisun2000 commented on a change in pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493781259
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -105,7 +106,9 @@
@PreFetchChangedData(enabled = false)
public class CallbackHandler implements IZkChildListener, IZkDataListener {
private static Logger logger = LoggerFactory.getLogger(CallbackHandler.class);
+ private static AtomicLong CB_UID = new AtomicLong();
Review comment:
changed.
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r488322127
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
Review comment:
Same thing here, it seems that directly refer to _uid is enough.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -660,24 +666,31 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
//TODO: avoid calling getChildren for path that does not exist
if (_changeType == CUSTOMIZED_STATE_ROOT) {
logger.warn(
- "Failed to subscribe child/data change on path: {}, listener: {}. Instance "
- + "does not support Customized State!", path, _listener);
+ "CallbackHandler {}, Failed to subscribe child/data change on path: {}, listener: {}. Instance "
+ + "does not support Customized State!", this.toString(), path, _listener);
} else {
- logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
- _listener, e);
+ logger.warn("CallbackHandler {}, Failed to subscribe child/data change. path: {}, listener: {}",
+ this.toString(), path, _listener, e);
}
}
}
}
long end = System.currentTimeMillis();
- logger.info("Subscribing to path: {} took: {}", path, (end - start));
+ logger.info("CallbackHandler{}, Subscribing to path: {} took: {}", this.toString(), path, (end - start));
}
public EventType[] getEventTypes() {
return (EventType[]) _eventTypes.toArray();
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
+ return stringBuilder.toString();
Review comment:
I agree to what Huizhi mentioned, please include the basic information. If only uid is needed, then just refer to the _uid field in the code. No need to override toString().
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -220,7 +227,7 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
_asyncCallRetryThread = new ZkAsyncRetryThread(zkConnection.getServers());
_asyncCallRetryThread.start();
- LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", _uid, _asyncCallRetryThread.getId());
+ LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", this.toString(), _asyncCallRetryThread.getId());
Review comment:
nit "_uid" => "uid"
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
Review comment:
I just notice that _uid here was not made final field, could you please fix it? It should be a final field.
----------------------------------------------------------------
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 pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1355:
URL: https://github.com/apache/helix/pull/1355#issuecomment-697947464
This pull request is approved, please help to merge
>CallbackHandlder and ZkClient methods are invoked over multiple threads.
It can be hard to correlate one object's execution thread over multiple
java threads. This poses a serious issue in production log examination.
We propose to add unique id to each methods' logging to help the
correlation and also track the object's life cycle.
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r486518801
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -660,24 +666,31 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
//TODO: avoid calling getChildren for path that does not exist
if (_changeType == CUSTOMIZED_STATE_ROOT) {
logger.warn(
- "Failed to subscribe child/data change on path: {}, listener: {}. Instance "
- + "does not support Customized State!", path, _listener);
+ "CallbackHandler {}, Failed to subscribe child/data change on path: {}, listener: {}. Instance "
+ + "does not support Customized State!", this.toString(), path, _listener);
} else {
- logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
- _listener, e);
+ logger.warn("CallbackHandler {}, Failed to subscribe child/data change. path: {}, listener: {}",
+ this.toString(), path, _listener, e);
}
}
}
}
long end = System.currentTimeMillis();
- logger.info("Subscribing to path: {} took: {}", path, (end - start));
+ logger.info("CallbackHandler{}, Subscribing to path: {} took: {}", this.toString(), path, (end - start));
}
public EventType[] getEventTypes() {
return (EventType[]) _eventTypes.toArray();
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
+ return stringBuilder.toString();
Review comment:
I think Lei mentioned in the review meeting not to hard code the UID. How about let me use a function called getUID()?
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493233983
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -28,6 +28,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
Review comment:
This is not used. Remove it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493221149
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2228,7 +2233,7 @@ public void close() throws ZkInterruptedException {
_eventThread.interrupt();
_eventThread.join(2000);
if (isManagingZkConnection()) {
- LOG.info("Closing zkclient: " + ((ZkConnection) connection).getZookeeper());
+ LOG.info("zkclient{}, Closing zkclient zk: {} ", this._uid, ((ZkConnection) connection).getZookeeper());
Review comment:
It'd be great to make these similar messages easier to read, like `Closing zkclient. uid={}, zk={}`?
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -105,7 +106,9 @@
@PreFetchChangedData(enabled = false)
public class CallbackHandler implements IZkChildListener, IZkDataListener {
private static Logger logger = LoggerFactory.getLogger(CallbackHandler.class);
+ private static AtomicLong CB_UID = new AtomicLong();
Review comment:
`static final`?
And also give it a more comprehensive name, like `CALLBACK_HANDLER_UID` or `NEXT_ID`?
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493909785
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -354,10 +359,10 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
// async mode only applicable to CALLBACK from ZK, During INIT and FINALIZE invoke the
// callback's immediately.
if (_batchModeEnabled && changeContext.getType() == NotificationContext.Type.CALLBACK) {
- logger.debug("Enqueuing callback");
+ logger.debug("Callbackhandler {}, Enqueuing callback", this._uid );
if (!isReady()) {
- logger.info("CallbackHandler is not ready, ignore change callback from path: {}, for "
- + "listener: {}", _path, _listener);
+ logger.info("CallbackHandler {} is not ready, ignore change callback from path: {}, for "
+ + "listener: {}", this._uid, _path, _listener);
Review comment:
removed all 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.
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r492467221
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
Review comment:
changed.
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r485325108
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -660,24 +666,31 @@ private void subscribeForChanges(NotificationContext.Type callbackType, String p
//TODO: avoid calling getChildren for path that does not exist
if (_changeType == CUSTOMIZED_STATE_ROOT) {
logger.warn(
- "Failed to subscribe child/data change on path: {}, listener: {}. Instance "
- + "does not support Customized State!", path, _listener);
+ "CallbackHandler {}, Failed to subscribe child/data change on path: {}, listener: {}. Instance "
+ + "does not support Customized State!", this.toString(), path, _listener);
} else {
- logger.warn("Failed to subscribe child/data change. path: {}, listener: {}", path,
- _listener, e);
+ logger.warn("CallbackHandler {}, Failed to subscribe child/data change. path: {}, listener: {}",
+ this.toString(), path, _listener, e);
}
}
}
}
long end = System.currentTimeMillis();
- logger.info("Subscribing to path: {} took: {}", path, (end - start));
+ logger.info("CallbackHandler{}, Subscribing to path: {} took: {}", this.toString(), path, (end - start));
}
public EventType[] getEventTypes() {
return (EventType[]) _eventTypes.toArray();
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
+ return stringBuilder.toString();
Review comment:
I think a `toString()` would give a comprehensive meaning/description for an object. Here, a `uid` doesn't achieve the purpose. If you just want to log uid, then put uid in log is enough.
If I override a toString(), I would add more descriptions/fields for this object, eg: "CallbackHandler[uid=xxx, path=xxx, ...]".
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r492467105
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -220,7 +227,7 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
_asyncCallRetryThread = new ZkAsyncRetryThread(zkConnection.getServers());
_asyncCallRetryThread.start();
- LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", _uid, _asyncCallRetryThread.getId());
+ LOG.debug("ZkClient created with _uid {}, _asyncCallRetryThread id {}", this.toString(), _asyncCallRetryThread.getId());
Review comment:
changed,
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -204,6 +204,13 @@ public void recordPathStat(Stat stat, OptionalLong notificationTime) {
}
}
+ @Override
+ public String toString() {
+ StringBuilder stringBuilder = new StringBuilder();
+ stringBuilder.append(_uid);
Review comment:
changed to final
----------------------------------------------------------------
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] pkuwm merged pull request #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
pkuwm merged pull request #1355:
URL: https://github.com/apache/helix/pull/1355
----------------------------------------------------------------
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 #1355: Enhance logging for CallbackHandler and Zkclient
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1355:
URL: https://github.com/apache/helix/pull/1355#discussion_r493901815
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -354,10 +359,10 @@ public void enqueueTask(NotificationContext changeContext) throws Exception {
// async mode only applicable to CALLBACK from ZK, During INIT and FINALIZE invoke the
// callback's immediately.
if (_batchModeEnabled && changeContext.getType() == NotificationContext.Type.CALLBACK) {
- logger.debug("Enqueuing callback");
+ logger.debug("Callbackhandler {}, Enqueuing callback", this._uid );
if (!isReady()) {
- logger.info("CallbackHandler is not ready, ignore change callback from path: {}, for "
- + "listener: {}", _path, _listener);
+ logger.info("CallbackHandler {} is not ready, ignore change callback from path: {}, for "
+ + "listener: {}", this._uid, _path, _listener);
Review comment:
nit, "this." in "this._uid" is not necessary, right?
Same as the following places.
----------------------------------------------------------------
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