KAFKA-17168: Remove the logPrefix to print the thread name#16657
Merged
chia7712 merged 3 commits intoapache:trunkfrom Jul 24, 2024
Merged
KAFKA-17168: Remove the logPrefix to print the thread name#16657chia7712 merged 3 commits intoapache:trunkfrom
chia7712 merged 3 commits intoapache:trunkfrom
Conversation
User can configure the log4j conversion-pattern to print the thread name if required.
kamalcph
changed the title
3 tasks
satishd
requested changes
Jul 22, 2024
|
|
||
| public class RemoteStorageThreadPool extends ThreadPoolExecutor { | ||
| private final Logger logger; | ||
| private final Logger logger = LoggerFactory.getLogger(RemoteStorageThreadPool.class); |
Member
There was a problem hiding this comment.
Can you make this as static to avoid creating multiple logger instances as we do not need any context for each instance? I understand that we create only a single instance for now but it is good to make this logger static even if we create multiple instances in future.
Member
There was a problem hiding this comment.
I understand that we create only a single instance for now but it is good to make this logger static even if we create multiple instances in future.
that is a good point. The default provider reload4j maintain the logger instance for each class name, so the instance should be same even though we don't use static. However, it seems slf4j docs does not describe such behavior so we can't assume other providers are same
for another, we should apply the static to code base. 1) unify the code style and 2) avoid multiple logger instances in the future.
chia7712
approved these changes
Jul 23, 2024
Member
|
@kamalcph could you please fix the build error? it seems we have to rename "logger" to "LOGGER" |
Contributor
Author
|
Test failures are unrelated. |
3 tasks
chia7712
pushed a commit
that referenced
this pull request
Jul 30, 2024
…ances (#16680) As discussed in #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>
abhi-ksolves
pushed a commit
to ksolves/kafka
that referenced
this pull request
Jul 31, 2024
Reviewers: Kuan-Po (Cooper) Tseng <brandboat@gmail.com>, Satish Duggana <satishd@apache.org>, Chia-Ping Tsai <chia7712@gmail.com>
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>
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
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.
Users can configure the log4j conversion-pattern to print the thread name if required. It is a followup of the comment.
Committer Checklist (excluded from commit message)