Skip to content

KAFKA-15853: Refactor ShareGroupConfig with AbstractConfig#16506

Merged
chia7712 merged 5 commits intoapache:trunkfrom
OmniaGM:KAFKA-15853-Refactor-ShareGroupConfig
Jul 10, 2024
Merged

KAFKA-15853: Refactor ShareGroupConfig with AbstractConfig#16506
chia7712 merged 5 commits intoapache:trunkfrom
OmniaGM:KAFKA-15853-Refactor-ShareGroupConfig

Conversation

@OmniaGM
Copy link
Copy Markdown
Contributor

@OmniaGM OmniaGM commented Jul 2, 2024

Move getters and validation out of KafkaConfig and into ShareGroupConfig

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@OmniaGM
Copy link
Copy Markdown
Contributor Author

OmniaGM commented Jul 2, 2024

@chia7712 can you have a look please whenever you have time? I started with this as the KIP-932 is under development at the moment and I don't want to clash with it

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jul 2, 2024

@OmniaGM that refactor is good, but maybe we can reach more consensus before refactor. Please take a look at #16458 (comment)

It would be nice we can build a pattern for all xxxConfig files.

@chia7712
Copy link
Copy Markdown
Member

chia7712 commented Jul 4, 2024

@OmniaGM as the summary of #16458 (comment), if the ShareGroupConfig is not dynamic we can introduce local attributes and validate all of them in construction. That can make sure each ShareGroupConfig is totally valid.

@OmniaGM
Copy link
Copy Markdown
Contributor Author

OmniaGM commented Jul 9, 2024

@OmniaGM as the summary of #16458 (comment), if the ShareGroupConfig is not dynamic we can introduce local attributes and validate all of them in construction. That can make sure each ShareGroupConfig is totally valid.

Just pushed a refactor here 5b4b862 to introduce attributes and move the validation to the construction. Please have a look when you have time.

Copy link
Copy Markdown
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.

@OmniaGM thanks for this patch. only two small comments 😄

Comment thread server/src/main/java/org/apache/kafka/server/config/ShareGroupConfig.java Outdated
Comment thread server/src/main/java/org/apache/kafka/server/config/ShareGroupConfig.java Outdated
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

lgtm. I was wondering how the refactoring that @OmniaGM is doing for config would affect the work we are doing on KIP-932.

@OmniaGM
Copy link
Copy Markdown
Contributor Author

OmniaGM commented Jul 10, 2024

lgtm. I was wondering how the refactoring that @OmniaGM is doing for config would affect the work we are doing on KIP-932.

@AndrewJSchofield I believe it shouldn't have significant effect on KIP-932's work apart from the fact that now all getters, validation and config definitions will be defined in ShareGroupConfig. Specially that as far as I can see we still didn't define all broker configs defined in KIP-932 yet. This also should reduce conflicts between refactoring KafkaConfig and broker config part of KIP-932.

Small suggestion you can depends on ShareGroupConfig instead of the full KafkaConfig where config is needed now if you wish.

@chia7712 chia7712 merged commit 25d775b into apache:trunk Jul 10, 2024
@apoorvmittal10
Copy link
Copy Markdown
Contributor

Thanks for the PR @OmniaGM, I do see GroupCoordinatorConfig also follows new way for configs 👍

abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 2024
Reviewers: Andrew Schofield <aschofield@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants