New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CURATOR-526 drop log level for message to debug #382
Conversation
ZooKeeper is backwards compatible and accepts older config string formats, albeit without dynamic configuration support. Since the older format is acceptable, we should not log at error when we see the older format and should instead simply log at debug.
I'd love to get this into 4.x as well as the current 5.x branch, appreciate guidance on how to make that happen! |
@@ -211,7 +211,7 @@ private void processConfigData(byte[] data) throws Exception | |||
} | |||
else | |||
{ | |||
log.error("Invalid config event received: {}", properties); | |||
log.debug("Invalid config event received: {}", properties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is the noise of the error level ?
if the problem is the log level, then we can use "info"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed the noise. I picked debug
over info
because the log message at 219 below is for a similar case (missing config) and is at debug
, but happy to change it to info
instead, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you paste a sample line of the log you are seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example:
Invalid config event received: {server.1=100.1.2.3:2888:3888:participant, version=0, server.5=100.2.3.4:2888:3888:participant, server.4=100.3.4.5:2888:3888:participant, server.3=100.4.5.6:2888:3888:participant, server.2=100.5.6.7:2888:3888:participant}
There's a bit more discussion on the Jira ticket, in case you haven't seen that yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringBuilder sb = new StringBuilder();
Iterator var2 = data.getAllMembers().values().iterator();
while(var2.hasNext()) {
QuorumServer server = (QuorumServer)var2.next();
if (server.clientAddr != null) {
if (sb.length() != 0) {
sb.append(",");
}
String hostAddress;
if (server.clientAddr.getAddress().isAnyLocalAddress()) {
hostAddress = Compatibility.getHostAddress(server);
} else {
hostAddress = server.clientAddr.getAddress().getHostAddress();
}
sb.append(hostAddress).append(":").append(server.clientAddr.getPort());
}
}
return sb.toString();
}```
- I think it is processing the **version=0** like another server more when it shouldn't.
- Do we know if this is affecting any functionality or not?
+1 to this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Committer to master branch. |
ZooKeeper is backwards compatible and accepts older config
string formats, albeit without dynamic configuration
support. Since the older format is acceptable, we should not
log at error when we see the older format and should
instead simply log at debug.