Skip to content

KAFKA-19980: ConfigCommand#validatePropsKey should accept $ symbol #21125

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.

Sign up for GitHub

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

Jump to bottom
Merged
chia7712 merged 2 commits into from
Dec 12, 2025
Merged

KAFKA-19980: ConfigCommand#validatePropsKey should accept $ symbol #21125

chia7712 merged 2 commits into from
Dec 12, 2025
+64 −17

Conversation

@DL1231
Copy link
Collaborator

@DL1231 DL1231 commented Dec 11, 2025 •
edited by github-actions bot
Loading

  1. Enabled ConfigCommand#validatePropsKey to support the $ symbol.
  2. Added UT and IT for the change.

Reviewers: Chih-Yuan Chien [email protected], Chia-Ping Tsai
[email protected]

@github-actions github-actions bot added triage PRs from the community core Kafka Broker tools small Small PRs labels Dec 11, 2025
chia7712
chia7712 reviewed Dec 11, 2025
View reviewed changes
chia7712
chia7712 approved these changes Dec 11, 2025
View reviewed changes
Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

@chia7712 chia7712 requested a review from smjn December 11, 2025 14:20
@chia7712
Copy link
Member

chia7712 commented Dec 11, 2025

@smjn Do you have a chance to take a look at this PR?

joshua2519
joshua2519 approved these changes Dec 12, 2025
View reviewed changes
Copy link
Contributor

@joshua2519 joshua2519 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this patch.

@github-actions github-actions bot removed the triage PRs from the community label Dec 12, 2025
@chia7712
Copy link
Member

chia7712 commented Dec 12, 2025

I am about to merge this. Since it is being shipped in trunk, we will have time to address any feedback in a follow-up PR

@chia7712 chia7712 merged commit 06a744a into apache:trunk Dec 12, 2025
25 checks passed
@kamalcph
Copy link
Contributor

kamalcph commented Dec 12, 2025

Thanks for fixing this issue!

shashankhs11 pushed a commit to shashankhs11/kafka that referenced this pull request Dec 15, 2025
@DL1231 @shashankhs11
KAFKA-19980 ConfigCommand#validatePropsKey should accept $ symbol (a…
8f5c47f 
…pache#21125)

1. Enabled ConfigCommand#validatePropsKey to support the `$` symbol.
2. Added UT and IT for the change.

Reviewers: Chih-Yuan Chien <[email protected]>, Chia-Ping Tsai
 <[email protected]>
@kamalcph
Copy link
Contributor

kamalcph commented Dec 17, 2025 •
edited
Loading

It is good to document this change for users. When running from CLI, we have to include a trailing slash to escape the dollar:

The below one will change the logger level in ClientQuotaManager class not in ThrottledChannelReaper:

sh kafka-configs.sh --bootstrap-server localhost:9092 --broker-logger 0 --add-config org.apache.kafka.server.quota.ClientQuotaManager$ThrottledChannelReaper=DEBUG --alter

Valid command:

sh kafka-configs.sh --bootstrap-server localhost:9092 --broker-logger 0 --add-config org.apache.kafka.server.quota.ClientQuotaManager\$ThrottledChannelReaper=DEBUG --alter

Feel free to ignore, added this comment for user reference when they hit/check this PR.

@chia7712
Copy link
Member

chia7712 commented Dec 17, 2025

It is good to document this change for users. When running from CLI, we have to include a trailing slash to escape the dollar:

That makes sense. @DL1231 Could you address this comment with a minor patch?

@DL1231 DL1231 mentioned this pull request Dec 20, 2025
Merged
@DL1231
Copy link
Collaborator Author

DL1231 commented Dec 20, 2025 •
edited
Loading

@kamalcph @chia7712
Thanks for the tip! I've added a comment in the code to highlight the need for escaping the $ symbol in CLI usage. This should help users understand the context.
PR: #21186

chia7712 pushed a commit that referenced this pull request Dec 22, 2025
@DL1231
MINOR: Add comment about escaping '$' symbol in ConfigCommand (#21186)
bb300ef 
see #21125 (comment),
this is a follow-up to KAFKA-19980.

It adds a clarifying comment in `ConfigCommand` to note that the '$'
symbol   must be escaped (e.g., `\$`) when used via the CLI

Reviewers: Kamal Chandraprakash <[email protected]>,
 Chia-Ping Tsai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

@smjn smjn Awaiting requested review from smjn

+1 more reviewer

@joshua2519 joshua2519 joshua2519 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core Kafka Broker small Small PRs tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@DL1231 @chia7712 @kamalcph @joshua2519