KAFKA-17081 Tweak GroupCoordinatorConfig: re-introduce local attributes and validation by brandboat · Pull Request #16524 · apache/kafka

brandboat · 2024-07-04T16:33:28Z

related to https://issues.apache.org/jira/browse/KAFKA-17081

address comments in #16458 (comment)

Committer Checklist (excluded from commit message)

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

…es and validation

chia7712

@brandboat thanks for this patch.

chia7712 · 2024-07-04T16:53:46Z

+import static org.apache.kafka.common.utils.Utils.require;

 /**
 * The group coordinator configurations.


please add comments why we create local attributes

chia7712 · 2024-07-04T16:59:25Z

+        this.consumerGroupSessionTimeoutMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_SESSION_TIMEOUT_MS_CONFIG);
+        this.consumerGroupHeartbeatIntervalMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_HEARTBEAT_INTERVAL_MS_CONFIG);
+        this.consumerGroupMaxSize = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_MAX_SIZE_CONFIG);
+        this.consumerGroupAssignors = config.getConfiguredInstances(GroupCoordinatorConfig.CONSUMER_GROUP_ASSIGNORS_CONFIG, ConsumerGroupPartitionAssignor.class);


could you add test to make sure incorrect value can cause error since CONSUMER_GROUP_ASSIGNORS_CONFIG` has no validator?

@brandboat Have you added test for CONSUMER_GROUP_ASSIGNORS_CONFIG?

For another, could you make consumerGroupAssignors immutable?

@brandboat Have you added test for CONSUMER_GROUP_ASSIGNORS_CONFIG?

I think It's already there before I submit this pr.

For another, could you make consumerGroupAssignors immutable?

Will do

chia7712 · 2024-07-04T17:01:08Z

+        this.offsetCommitTimeoutMs = config.getInt(GroupCoordinatorConfig.OFFSET_COMMIT_TIMEOUT_MS_CONFIG);
+        this.consumerGroupMigrationPolicy = ConsumerGroupMigrationPolicy.parse(
+                config.getString(GroupCoordinatorConfig.CONSUMER_GROUP_MIGRATION_POLICY_CONFIG));
+        this.offsetTopicCompressionType = Optional.ofNullable(config.getInt(GroupCoordinatorConfig.OFFSETS_TOPIC_COMPRESSION_CODEC_CONFIG))


ditto. please add test. Also, maybe we should add validator to the def of OFFSETS_TOPIC_COMPRESSION_CODEC_CONFIG

chia7712 · 2024-07-04T17:03:25Z

+        this.offsetsRetentionCheckIntervalMs = config.getLong(GroupCoordinatorConfig.OFFSETS_RETENTION_CHECK_INTERVAL_MS_CONFIG);
+        this.offsetsRetentionMs = config.getInt(GroupCoordinatorConfig.OFFSETS_RETENTION_MINUTES_CONFIG) * 60L * 1000L;
+        this.offsetCommitTimeoutMs = config.getInt(GroupCoordinatorConfig.OFFSET_COMMIT_TIMEOUT_MS_CONFIG);
+        this.consumerGroupMigrationPolicy = ConsumerGroupMigrationPolicy.parse(


Thanks ! I fixed all of them.

Please add UT to ensure the incorrect value can cause in contructor.

All comments are addressed in the latest commit, thanks

chia7712

@brandboat thanks for updated patch

chia7712 · 2024-07-05T22:46:47Z

+        this.consumerGroupSessionTimeoutMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_SESSION_TIMEOUT_MS_CONFIG);
+        this.consumerGroupHeartbeatIntervalMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_HEARTBEAT_INTERVAL_MS_CONFIG);
+        this.consumerGroupMaxSize = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_MAX_SIZE_CONFIG);
+        this.consumerGroupAssignors = config.getConfiguredInstances(GroupCoordinatorConfig.CONSUMER_GROUP_ASSIGNORS_CONFIG, ConsumerGroupPartitionAssignor.class);


@brandboat Have you added test for CONSUMER_GROUP_ASSIGNORS_CONFIG?

chia7712 · 2024-07-05T22:47:05Z

+        this.consumerGroupSessionTimeoutMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_SESSION_TIMEOUT_MS_CONFIG);
+        this.consumerGroupHeartbeatIntervalMs = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_HEARTBEAT_INTERVAL_MS_CONFIG);
+        this.consumerGroupMaxSize = config.getInt(GroupCoordinatorConfig.CONSUMER_GROUP_MAX_SIZE_CONFIG);
+        this.consumerGroupAssignors = config.getConfiguredInstances(GroupCoordinatorConfig.CONSUMER_GROUP_ASSIGNORS_CONFIG, ConsumerGroupPartitionAssignor.class);


For another, could you make consumerGroupAssignors immutable?

chia7712 · 2024-07-05T22:48:11Z

+        this.offsetsRetentionCheckIntervalMs = config.getLong(GroupCoordinatorConfig.OFFSETS_RETENTION_CHECK_INTERVAL_MS_CONFIG);
+        this.offsetsRetentionMs = config.getInt(GroupCoordinatorConfig.OFFSETS_RETENTION_MINUTES_CONFIG) * 60L * 1000L;
+        this.offsetCommitTimeoutMs = config.getInt(GroupCoordinatorConfig.OFFSET_COMMIT_TIMEOUT_MS_CONFIG);
+        this.consumerGroupMigrationPolicy = ConsumerGroupMigrationPolicy.parse(


Please add UT to ensure the incorrect value can cause in contructor.

…es and validation (apache#16524) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

KAFKA-17081 Tweak GroupCoordinatorConfig: re-introduce local attribut…

772b516

…es and validation

chia7712 reviewed Jul 4, 2024

View reviewed changes

brandboat added 2 commits July 5, 2024 23:46

Address comments

8c400cc

Merge remote-tracking branch 'origin/trunk' into KAFKA-17081

7c00722

chia7712 reviewed Jul 5, 2024

View reviewed changes

brandboat added 2 commits July 6, 2024 11:52

Address comments

9b5aa83

Merge remote-tracking branch 'origin/trunk' into KAFKA-17081

d336afc

chia7712 merged commit a533e24 into apache:trunk Jul 7, 2024

brandboat deleted the KAFKA-17081 branch July 8, 2024 00:16

abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 2024

KAFKA-17081 Tweak GroupCoordinatorConfig: re-introduce local attribut…

8abe750

…es and validation (apache#16524) Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

Conversation

brandboat commented Jul 4, 2024

Committer Checklist (excluded from commit message)

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants