HADOOP-17295 Move dedicated pre-logging statements into existing logg… #2358
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
steveloughran
requested changes
Oct 7, 2020
There was a problem hiding this comment.
This is potentially useful, though you should know we are scared of giant patches which span everything -they make backporting really hard. package-by-package would be better.
- a lot of the old logging was based on commons-logging APIs, there's a lot of concatenation there.
- toString() calls in logging is also needless and expensive, e.g
LOG.debug("status ={}", status.toString()) - needless operations before logging matter most for debug logging, as it is usually off by default. info/warn/error we don't need to guard
- At the same time, what matters there, especially for info, is probably the cost of the evaluation: its the cost of the toString and other calls which would be most beneficial
...anager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/DefaultContainerExecutor.java
Outdated
Show resolved
Hide resolved
...project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPServiceActor.java
Outdated
Show resolved
Hide resolved
|
@steveloughran Appreciate your review. Your comment on the logging overhead is valuable for me. I have another question that I would like to hear your thoughts. Do you prefer to reduce accumulated overhead or average overhead? The point is that some simple logging calls are on the hot path and frequently executed. The accumulated overhead for these simple logging calls is larger than other complex logging calls rarely executed. |
|
💔 -1 overall
This message was automatically generated. |
personally
|
|
catching up on my long-neglected review homework. @ccoder-chenzhi can you rebase this to trunk? |
coder-chenzhi
force-pushed
the
trunk
branch
2 times, most recently
from
ea7ff2b to
f8e6423
Compare
|
@steveloughran This PR is rebased to trunk and ready for merge. |
|
💔 -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
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.
I find some cases where some pre-processing statements dedicated to logging calls are not guarded by existing logging guards. Most of them are easy to fix. And the performance and maintainability of these logging calls can be improved to some extend. So I create this PR to fix them. I guess there is no need to add new test cases.
These issues are detected by a static analysis tool wrote by myself. This tool can extract all the dedicated statements for each debug-logging calls (i.e., the results of these statements are only used by debug-logging calls). Because I realize that debug logs will incur overhead in production, such as string concatenation and method calls in the parameters of logging calls. And I want to perform a systematic evaluation for the overhead of debugging logging calls in production.