KAFKA-17185: Declare Loggers as static to prevent multiple logger instances#16680
Merged
chia7712 merged 7 commits intoapache:trunkfrom Jul 30, 2024
Conversation
mingyen066
changed the title
chia7712
reviewed
Jul 24, 2024
| static final double CONNECTION_SETUP_TIMEOUT_JITTER = 0.2; | ||
| private final Map<String, NodeConnectionState> nodeState; | ||
| private final Logger log; | ||
| private static final Logger log; |
Member
There was a problem hiding this comment.
Noted that this static variable should NOT be initialized in constructor.
Collaborator
Author
There was a problem hiding this comment.
Thanks for finding the bug. This is the case that I shouldn't change the logger to static.
I have completed the code updates with different approach and also written the methodology and criteria used to PR message. PTAL.
…le-logger-instances' of https://github.com/mingyen066/kafka into KAFKA-17185-Make-logger-static-to-avoid-creating-multiple-logger-instances
…instances" This reverts commit 00b6317.
mingyen066
changed the title
…ogger-static-to-avoid-creating-multiple-logger-instances
Member
|
@mingyen066 Could you please merge trunk to run CI again? there are thread leak. I don't think there are related to your PR, but it is always good to check it again |
…ogger-static-to-avoid-creating-multiple-logger-instances
Collaborator
Author
|
@chia7712 Sure, I merged trunk branch and the CI is running again. |
abhi-ksolves
pushed a commit
to ksolves/kafka
that referenced
this pull request
Jul 31, 2024
…ances (apache#16680) As discussed in apache#16657 (comment) , we should make logger as static to avoid creating multiple logger instances. I use the regex private.*Logger.*LoggerFactory to search and check all the results if certain logs need to be static. There are some exceptions that loggers don't need to be static: 1) The logger in the inner class. Since java8 doesn't support static field in the inner class. https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/clients/consumer/internals/FetchRequestManagerTest.java#L3676 2) Custom loggers for each instance (non-static + non-final). In this case, multiple logger instances is actually really needed. https://github.com/apache/kafka/blob/trunk/storage/src/test/java/org/apache/kafka/server/log/remote/storage/LocalTieredStorage.java#L166 3) The logger is initialized in constructor by LogContext. Many non-static but with final modifier loggers are in this category, that's why I use .*LoggerFactory to only check the loggers that are assigned initial value when declaration. 4) protected final Logger log = Logger.getLogger(getClass()) This is for subclass can do logging with subclass name instead of superclass name. But in this case, if the log access modifier is private, the purpose cannot be achieved since subclass cannot access the log defined in superclass. So if access modifier is private, we can replace getClass() with <className>.class Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
mingyen066
deleted the
KAFKA-17185-Make-logger-static-to-avoid-creating-multiple-logger-instances
branch
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
As discussed in #16657 (comment) , we should make logger as static to avoid creating multiple logger instances.
I use the regex
private.*Logger.*LoggerFactoryto search and check all the results if certain logs need to be static.There are some exceptions that loggers don't need to be static:
.*LoggerFactoryto only check the loggers that are assigned initial value when declaration.protected final Logger log = Logger.getLogger(getClass())This is for subclass can do logging with subclass name instead of superclass name.
But in this case, if the log access modifier is private, the purpose cannot be achieved since subclass cannot access the log defined in superclass. So if access modifier is private, we can replace
getClass()with<className>.classCommitter Checklist (excluded from commit message)