-
Notifications
You must be signed in to change notification settings - Fork 14.3k
KAFKA-17821: the set of configs displayed by logAll
could be invalid
#17993
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
base: trunk
Are you sure you want to change the base?
Conversation
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.
Thanks @m1a2st for the PR.
It appears that the overridden version of clearUnsupportedConfigsForLogging()
doesn't return a Map
of type TreeMap
. The result would be that the entries wouldn't be sorted.
Also, are there any unit tests that could be written to verify this change?
Thanks!
if (doLog) | ||
logAll(); | ||
if (doLog) { | ||
Map<String, Object> loggingConfig = clearUnsupportedConfigsForLogging(this.values); | ||
logAll(loggingConfig); | ||
} |
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.
Is it possible to invoke clearUnsupportedConfigsForLogging()
inside logAll()
and thus the method signature can remain as is?
return new TreeMap<>(values); | ||
} | ||
|
||
private void logAll(Map<String, Object> values) { | ||
StringBuilder b = new StringBuilder(); | ||
b.append(getClass().getSimpleName()); | ||
b.append(" values: "); | ||
b.append(Utils.NL); | ||
|
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.
That way we can invoke this from inside logAll()
, keeping the logging bits closer together.
Map<String, Object> valuesToLog - clearUnsupportedConfigsForLogging(values); |
Thanks for @kirktrue review, addressed all comments. |
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
Jira: https://issues.apache.org/jira/browse/KAFKA-17821
User can config different protocol in consumer config. We will print all config in log, but some configs has the default value and is unsupported for each protocol, thus we print these config will misdirect user.
We should improve this, and won't show the unsupported configs in log.
test in my local, if use CLASSIC protocol won't show
group.remote.assignor
Committer Checklist (excluded from commit message)