chia7712 commented on code in PR #21593:
URL: https://github.com/apache/kafka/pull/21593#discussion_r2885555017
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorConfig.java:
##########
@@ -647,12 +647,13 @@ protected List<ShareGroupPartitionAssignor>
shareGroupAssignors(
}
/**
- * Copy the subset of properties that are relevant to consumer group and
share group.
+ * Copy the subset of properties that are relevant to consumer group,
share group and streams group.
*/
public Map<String, Integer> extractGroupConfigMap(ShareGroupConfig
shareGroupConfig) {
Map<String, Integer> defaultConfigs = new HashMap<>();
defaultConfigs.putAll(extractConsumerGroupConfigMap());
defaultConfigs.putAll(shareGroupConfig.extractShareGroupConfigMap(this));
Review Comment:
Could you move `extractShareGroupConfigMap` from `ShareGroupConfig` to
`GroupCoordinatorConfig` for consistency?
```java
public Map<String, Integer> extractShareGroupConfigMap(ShareGroupConfig
shareGroupConfig) {
return Map.of(
GroupConfig.SHARE_SESSION_TIMEOUT_MS_CONFIG,
this.shareGroupSessionTimeoutMs(),
GroupConfig.SHARE_HEARTBEAT_INTERVAL_MS_CONFIG,
this.shareGroupHeartbeatIntervalMs(),
GroupConfig.SHARE_RECORD_LOCK_DURATION_MS_CONFIG,
shareGroupConfig.shareGroupRecordLockDurationMs(),
GroupConfig.SHARE_DELIVERY_COUNT_LIMIT_CONFIG,
shareGroupConfig.shareGroupDeliveryCountLimit()
);
}
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupConfigManager.java:
##########
@@ -76,17 +76,18 @@ public List<String> groupIds() {
/**
* Validate the given properties.
*
- * @param newGroupConfig The new group config.
- * @param groupCoordinatorConfig The group coordinator config.
- * @throws InvalidConfigurationException If validation fails
+ * @param newGroupConfig The new group config.
+ * @param groupCoordinatorConfig The group coordinator config.
+ * @param shareGroupConfig The share group config.
+ * @throws InvalidConfigurationException If validation fails.
*/
public static void validate(
Properties newGroupConfig,
GroupCoordinatorConfig groupCoordinatorConfig,
ShareGroupConfig shareGroupConfig
) {
Properties combinedConfigs = new Properties();
-
combinedConfigs.putAll(groupCoordinatorConfig.extractConsumerGroupConfigMap());
+
combinedConfigs.putAll(groupCoordinatorConfig.extractGroupConfigMap(shareGroupConfig));
combinedConfigs.putAll(newGroupConfig);
GroupConfig.validate(combinedConfigs, groupCoordinatorConfig,
shareGroupConfig);
Review Comment:
Could we do the merge in `GroupConfig.validate` rather than
`GroupConfigManager.validate` ? With this change, we could even remove
`GroupConfigManager.validate` to streamline the call chain
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
