Skip to content

Conversation

@gongxuanzhang
Copy link
Contributor

@gongxuanzhang gongxuanzhang commented Apr 16, 2025

This PR is a umbrella of KAFKA-18854.

The previous PR encountered some compatibility issues, so we decided to
split it and proceed with the migration step by step.

see #19019

Reviewers: PoAn Yang payang@apache.org, Chia-Ping Tsai
chia7712@gmail.com

@github-actions github-actions bot added triage PRs from the community core Kafka Broker labels Apr 16, 2025
@chia7712
Copy link
Member

@gongxuanzhang please revise the title :)

@gongxuanzhang gongxuanzhang changed the title KAFKA-18854:Move DynamicConfig to server module KAFKA-18854: Move DynamicConfig to server module Apr 16, 2025
@gongxuanzhang
Copy link
Contributor Author

gongxuanzhang commented Apr 16, 2025

please revise the title :)

Thanks for your comment @chia7712 PTAL

@gongxuanzhang gongxuanzhang changed the title KAFKA-18854: Move DynamicConfig to server module KAFKA-18854: Move DynamicConfig User and DynamicConfig Client to server module Apr 16, 2025


public static class Client {
private static final ConfigDef CONFIG_DEF = QuotaConfig.userAndClientQuotaConfigs();
Copy link
Member

Choose a reason for hiding this comment

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

Could we rename this to CLIENT_CONFIGS, since it was clientConfigs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice comment

return USER_CONFIGS.names();
}

public static Map<String, Object> validate(Properties props) {
Copy link
Member

Choose a reason for hiding this comment

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

those method is useless, so please remove it.

import java.util.Properties;
import java.util.Set;

public class DynamicConfig {
Copy link
Member

Choose a reason for hiding this comment

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

those helpers are used by ConfigCommand only, so please remove inline this whole class.

@github-actions github-actions bot added small Small PRs and removed triage PRs from the community labels Apr 17, 2025
@gongxuanzhang
Copy link
Contributor Author

@chia7712 PTAL

@chia7712
Copy link
Member

@gongxuanzhang could you please cleanup IP, group, and ClientMetrics?

@gongxuanzhang gongxuanzhang changed the title KAFKA-18854: Move DynamicConfig User and DynamicConfig Client to server module KAFKA-18854: remove DynamicConfig inner class Apr 18, 2025
@gongxuanzhang
Copy link
Contributor Author

@chia7712 PTAL

@chia7712 chia7712 merged commit a78a931 into apache:trunk Apr 20, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-approved core Kafka Broker small Small PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants