Skip to content

Conversation

@showuon
Copy link
Member

@showuon showuon commented Oct 23, 2025

Improve some requests/responses toString method to log only the
required
info, including the request.Builder toString methods.

  1. AlterConfigsRequest
  2. AlterUserScramCredentialsRequest
  3. ExpireDelegationTokenRequest
  4. IncrementalAlterConfigsRequest
  5. RenewDelegationTokenRequest
  6. SaslAuthenticateRequest
  7. createDelegationTokenResponse
  8. describeDelegationTokenResponse
  9. SaslAuthenticateResponse

Reviewers: Chia-Ping Tsai [email protected], Manikumar Reddy
[email protected]

@github-actions github-actions bot added core Kafka Broker clients small Small PRs labels Oct 23, 2025
@showuon
Copy link
Member Author

showuon commented Oct 23, 2025

@ahuang98 @omkreddy , please take a look. Thanks.

// It is not safe to print all config values
@Override
public String toString() {
JsonNode json = AlterConfigsRequestDataJsonConverter.write(data, version()).deepCopy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment in the email discussion thread

@chia7712
Copy link
Member

@lianetm would you mind including this patch in the 4.1.1?

@github-actions github-actions bot removed the small Small PRs label Oct 24, 2025
@showuon showuon force-pushed the alterConfigToString branch from eacd2c8 to 26ffff4 Compare October 24, 2025 07:18
@showuon showuon changed the title MINOR: Improve Alter config request toString MINOR: Improve some requests toString method Oct 24, 2025
@showuon showuon changed the title MINOR: Improve some requests toString method MINOR: Improve some requests/responses toString method Oct 24, 2025
@lianetm
Copy link
Member

lianetm commented Oct 24, 2025

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!

return new AlterConfigsRequest.Builder(configs, false).build(version);
AlterConfigsRequest alterConfigsRequest = new AlterConfigsRequest.Builder(configs, false).build(version);
assertEquals(
"AlterConfigsRequestData(resources=[" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failed test is related to this, as the order of elements is not deterministic due to the HashMap

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I push a fix for this test

Copy link
Member Author

@showuon showuon Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @chia7712 ! LGTM! Let's wait for the CI build result.

@chia7712 chia7712 merged commit 23d6764 into apache:trunk Oct 25, 2025
23 checks passed
chia7712 pushed a commit that referenced this pull request Oct 25, 2025
Improve some requests/responses toString method to log only the
required
info, including the request.Builder toString methods.
1. AlterConfigsRequest
2. AlterUserScramCredentialsRequest
3. ExpireDelegationTokenRequest
4. IncrementalAlterConfigsRequest
5. RenewDelegationTokenRequest
6. SaslAuthenticateRequest
7. createDelegationTokenResponse
8. describeDelegationTokenResponse
9. SaslAuthenticateResponse

Reviewers: Chia-Ping Tsai <[email protected]>, Manikumar Reddy
 <[email protected]>
@chia7712
Copy link
Member

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

@brandboat
Copy link
Member

I’ll help Chia-Ping resolve the conflicts 😃

brandboat pushed a commit to brandboat/kafka that referenced this pull request Oct 25, 2025
Improve some requests/responses toString method to log only the
required
info, including the request.Builder toString methods.
1. AlterConfigsRequest
2. AlterUserScramCredentialsRequest
3. ExpireDelegationTokenRequest
4. IncrementalAlterConfigsRequest
5. RenewDelegationTokenRequest
6. SaslAuthenticateRequest
7. createDelegationTokenResponse
8. describeDelegationTokenResponse
9. SaslAuthenticateResponse

Reviewers: Chia-Ping Tsai <[email protected]>, Manikumar Reddy
 <[email protected]>
brandboat pushed a commit to brandboat/kafka that referenced this pull request Oct 25, 2025
Improve some requests/responses toString method to log only the
required
info, including the request.Builder toString methods.
1. AlterConfigsRequest
2. AlterUserScramCredentialsRequest
3. ExpireDelegationTokenRequest
4. IncrementalAlterConfigsRequest
5. RenewDelegationTokenRequest
6. SaslAuthenticateRequest
7. createDelegationTokenResponse
8. describeDelegationTokenResponse
9. SaslAuthenticateResponse

Reviewers: Chia-Ping Tsai <[email protected]>, Manikumar Reddy
 <[email protected]>
@brandboat
Copy link
Member

brandboat commented Oct 25, 2025

4.0 backport pr: #20775
3.9 backport pr: #20776
c.c. @chia7712

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clients core Kafka Broker

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants