You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "showuon (via GitHub)" <gi...@apache.org> on 2025/10/23 09:24:14 UTC
[PR] MINOR: Improve Alter config request toString [kafka]
showuon opened a new pull request, #20759:
URL: https://github.com/apache/kafka/pull/20759
Improve the AlterConfigRequest toString method to log only the required info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3446827783
I’ll help Chia-Ping resolve the conflicts 😃
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3446820974
I have pushed this patch to trunk and 4.1, but conflicts occurred in 4.0 and 3.9. will file a separate patch for those branches
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve Alter config request toString [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20759:
URL: https://github.com/apache/kafka/pull/20759#discussion_r2456033613
##########
clients/src/main/java/org/apache/kafka/common/requests/AlterConfigsRequest.java:
##########
@@ -134,4 +137,16 @@ public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
public static AlterConfigsRequest parse(Readable readable, short version) {
return new AlterConfigsRequest(new AlterConfigsRequestData(readable, version), version);
}
+
+ // It is not safe to print all config values
+ @Override
+ public String toString() {
+ JsonNode json = AlterConfigsRequestDataJsonConverter.write(data, version()).deepCopy();
Review Comment:
I left a comment in the email discussion thread
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20759:
URL: https://github.com/apache/kafka/pull/20759#discussion_r2460440125
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -3081,7 +3082,19 @@ private AlterConfigsRequest createAlterConfigsRequest(short version) {
configs.put(new ConfigResource(ConfigResource.Type.BROKER, "0"), new AlterConfigsRequest.Config(configEntries));
configs.put(new ConfigResource(ConfigResource.Type.TOPIC, "topic"),
new AlterConfigsRequest.Config(emptyList()));
- return new AlterConfigsRequest.Builder(configs, false).build(version);
+ AlterConfigsRequest alterConfigsRequest = new AlterConfigsRequest.Builder(configs, false).build(version);
+ assertEquals(
+ "AlterConfigsRequestData(resources=[" +
Review Comment:
The failed test is related to this, as the order of elements is not deterministic due to the `HashMap`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20759:
URL: https://github.com/apache/kafka/pull/20759#discussion_r2462681037
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -3081,7 +3082,19 @@ private AlterConfigsRequest createAlterConfigsRequest(short version) {
configs.put(new ConfigResource(ConfigResource.Type.BROKER, "0"), new AlterConfigsRequest.Config(configEntries));
configs.put(new ConfigResource(ConfigResource.Type.TOPIC, "topic"),
new AlterConfigsRequest.Config(emptyList()));
- return new AlterConfigsRequest.Builder(configs, false).build(version);
+ AlterConfigsRequest alterConfigsRequest = new AlterConfigsRequest.Builder(configs, false).build(version);
+ assertEquals(
+ "AlterConfigsRequestData(resources=[" +
Review Comment:
I push a fix for this test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3446873097
4.0 backport pr: https://github.com/apache/kafka/pull/20775
3.9 backport pr: https://github.com/apache/kafka/pull/20776
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve Alter config request toString [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3435930749
@ahuang98 @omkreddy , please take a look. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #20759:
URL: https://github.com/apache/kafka/pull/20759#discussion_r2462684151
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -3081,7 +3082,19 @@ private AlterConfigsRequest createAlterConfigsRequest(short version) {
configs.put(new ConfigResource(ConfigResource.Type.BROKER, "0"), new AlterConfigsRequest.Config(configEntries));
configs.put(new ConfigResource(ConfigResource.Type.TOPIC, "topic"),
new AlterConfigsRequest.Config(emptyList()));
- return new AlterConfigsRequest.Builder(configs, false).build(version);
+ AlterConfigsRequest alterConfigsRequest = new AlterConfigsRequest.Builder(configs, false).build(version);
+ assertEquals(
+ "AlterConfigsRequestData(resources=[" +
Review Comment:
Thanks @chia7712 ! LGTM!
##########
clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java:
##########
@@ -3081,7 +3082,19 @@ private AlterConfigsRequest createAlterConfigsRequest(short version) {
configs.put(new ConfigResource(ConfigResource.Type.BROKER, "0"), new AlterConfigsRequest.Config(configEntries));
configs.put(new ConfigResource(ConfigResource.Type.TOPIC, "topic"),
new AlterConfigsRequest.Config(emptyList()));
- return new AlterConfigsRequest.Builder(configs, false).build(version);
+ AlterConfigsRequest alterConfigsRequest = new AlterConfigsRequest.Builder(configs, false).build(version);
+ assertEquals(
+ "AlterConfigsRequestData(resources=[" +
Review Comment:
Thanks @chia7712 ! LGTM! Let's wait for the CI build result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve Alter config request toString [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3438076838
@lianetm would you mind including this patch in the 4.1.1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3448648287
Thanks all, I'm starting a new RC for 4.1.1 with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on PR #20759:
URL: https://github.com/apache/kafka/pull/20759#issuecomment-3443089860
Hey @chia7712 , this seems sensitive enough, so I wouldn't mind including it if we're confident it's not too disruptive. There are some related failures though, @showuon could you take a look (FAILED ❌ RequestResponseTest > testSerialization())
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Improve some requests/responses toString method [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20759:
URL: https://github.com/apache/kafka/pull/20759
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org