chia7712 commented on code in PR #21467:
URL: https://github.com/apache/kafka/pull/21467#discussion_r2876851661
##########
core/src/main/java/kafka/server/share/ShareFetchUtils.java:
##########
@@ -275,6 +276,20 @@ public static int
recordLockDurationMsOrDefault(GroupConfigManager groupConfigMa
return defaultValue;
}
+ /**
+ * The method is used to check if renew acknowledge is enabled for the
group. If the group config
+ * is present, then the value from the group config is used. Otherwise,
the default value is used.
+ *
+ * @param groupConfigManager The group config manager.
+ * @param groupId The group id for which the renew acknowledge enable is
to be checked.
+ * @return true if renew acknowledge is enabled for the group, false
otherwise.
+ */
+ public static boolean isRenewAcknowledgeEnabled(GroupConfigManager
groupConfigManager, String groupId) {
Review Comment:
I'm a bit concerned that `ShareFetchUtils` is accumulating too many helpers
and might eventually become a god object. Maybe we could introduces a
`ShareGroupConfigProvider` class to encapsulate these config-related helpers?
```java
public class ShareGroupConfigProvider {
private final GroupConfigManager manager;
public ShareGroupConfigProvider(GroupConfigManager manager) {
this.manager = manager;
}
public boolean isRenewAcknowledgeEnabled(String groupId) {
return manager.groupConfig(groupId)
.map(GroupConfig::shareRenewAcknowledgeEnable)
.orElse(GroupConfig.SHARE_RENEW_ACKNOWLEDGE_ENABLE_DEFAULT);
}
}
```
Also, would you mind refactoring `deliveryCountLimitOrDefault` and
`recordLockDurationMsOrDefault`? They don't use the fluent style, which could
potentially produce a `NoSuchElementException` if the config is removed
concurrently from the `GroupConfigManager`
```java
public static int recordLockDurationMsOrDefault(GroupConfigManager
groupConfigManager, String groupId, int defaultValue) {
if (groupConfigManager.groupConfig(groupId).isPresent()) {
// if the config gets removed here, it causes
NoSuchElementException
return
groupConfigManager.groupConfig(groupId).get().shareRecordLockDurationMs();
}
return defaultValue;
}
```
--
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]
