HBASE-26163 Better logging in RSGroupInfoManagerImpl #3610
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 21, 2021
| if (!fallbackRegions.isEmpty()) { | ||
| List<ServerName> candidates = null; | ||
| if (isFallbackEnabled()) { | ||
| LOG.debug("Falling back {} regions to servers outside their RSGroup. Regions: {}", |
There was a problem hiding this comment.
Better wrap it with a LOG.isDebugEnabled? We have a stream operation when constructing the parameter.
| LOG.info("Updated default servers, {} servers", newDefaultGroupInfo.getServers().size()); | ||
| LOG.info("Updated default servers, now {} servers online: {}", | ||
| newDefaultGroupInfo.getServers().size(), | ||
| newDefaultGroupInfo.getServers().stream().map(Address::toString).collect(Collectors.toSet())); |
There was a problem hiding this comment.
I think the message will be too long if we log the servers? This is a log in info level, not debug.
| // according to the inputted newGroupMap (an updated copy of rsGroupMap) | ||
| this.holder = new RSGroupInfoHolder(newGroupMap); | ||
|
|
||
| LOG.info("New RSGroup map: {}", newGroupMap.toString()); |
| resetRSGroupMap(newGroupMap); | ||
| saveRSGroupMapToZK(newGroupMap); | ||
| updateCacheOfRSGroups(newGroupMap.keySet()); | ||
| LOG.info("Flush config done, new RSGroup map: {}", newGroupMap.toString()); |
| ProcedureSyncWait.waitForProcedureToCompleteIOE(masterServices.getMasterProcedureExecutor(), | ||
| proc, Long.MAX_VALUE); | ||
| } | ||
| LOG.info("Move tables done. Moved {} tables to {}: {}", tables.size(), targetGroup, |
|
@Apache9 thanks for the review. I moved the lines you mentioned are too long to debug level logging instead. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bharathv
requested changes
Aug 24, 2021
| // according to the inputted newGroupMap (an updated copy of rsGroupMap) | ||
| this.holder = new RSGroupInfoHolder(newGroupMap); | ||
|
|
||
| LOG.debug("New RSGroup map: {}", newGroupMap.toString()); |
There was a problem hiding this comment.
Its not a good idea to call toString() on the parameter as that defeats the purpose of parameterized logging, if DEBUG is not enabled. toString() is implicitly evaluated by log4j only if DEBUG logging is enabled for this class. Please get rid of toString() here and below.
https://www.javacodegeeks.com/2015/10/better-performing-non-logging-logger-calls-in-log4j2.html
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 if no more comments, are we good to merge this one? |
Apache9
reviewed
Aug 25, 2021
| proc, Long.MAX_VALUE); | ||
| } | ||
| LOG.info("Move tables done. Moved {} tables to {}: {}", tables.size(), targetGroup, | ||
| tables.stream().map(TableName::getNameAsString).collect(Collectors.toSet())); |
There was a problem hiding this comment.
I think tableName.toString is the same with getNameAsString? So we do not need to generate a new String set?
| LOG.info("Move servers done: {} => {}", srcGrp.getName(), targetGroupName); | ||
| LOG.info("Move servers done. Moved {} servers {} => {}: {}", movedServers.size(), | ||
| srcGrp.getName(), targetGroupName, | ||
| movedServers.stream().map(Address::toString).collect(Collectors.toSet())); |
There was a problem hiding this comment.
Also here, I do not think we need to call toString explicitly...
|
@Apache9 thanks, removed the |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 26, 2021
| ProcedureSyncWait.waitForProcedureToCompleteIOE(masterServices.getMasterProcedureExecutor(), | ||
| proc, Long.MAX_VALUE); | ||
| } | ||
| LOG.info("Move tables done. Moved {} tables to {}: {}", tables.size(), targetGroup, tables); |
There was a problem hiding this comment.
We will log too much if the level is info here? Can we just info the size and the target group, and add another debug log to log all the tables?
| targetGroupName); | ||
| moveServerRegionsFromGroup(movedServers, srcGrp.getServers(), targetGroupName, srcGrp.getName()); | ||
| LOG.info("Move servers done: {} => {}", srcGrp.getName(), targetGroupName); | ||
| LOG.info("Move servers done. Moved {} servers from {} => {}: {}", movedServers.size(), |
There was a problem hiding this comment.
Ditto, I'm afraid it will too much to also log all the servers name, but log the size in info and then all the servers with debug.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 1, 2021
|
I'm OK with merging. Let's for one more day to see if @bharathv could come back and reply here. Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.