frankvicky commented on code in PR #22676:
URL: https://github.com/apache/kafka/pull/22676#discussion_r3491641158
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorShard.java:
##########
@@ -1014,7 +1015,7 @@ public Map<String, Integer>
listStreamsGroupsNeedingTopologyCleanup(long committ
if (!streamsGroup.isEmpty(committedOffset)) continue;
int storedEpoch =
streamsGroup.storedDescriptionTopologyEpoch(committedOffset);
Review Comment:
Like the direction, but I think the literal fold introduces a circular
dependency with `groupHasNoOffsets` simplification. Today's sweep order is:
```java
if (!group.shouldExpire()) return; // ShareGroup
short-circuits here
boolean allOffsetsExpired = cleanupExpiredOffsets(...); // tombstones
offsets
if (!allOffsetsExpired) return;
if (deferStreamsGroupTombstoneForPluginCleanup(...)) return; // streams gate
only blocks the tombstone
maybeDeleteGroup(...);
```
A deferred streams group still runs `cleanupExpiredOffsets` — only the
tombstone is skipped. That offset cleanup is the topology cycle's entry
condition: it watches for `groupHasNoOffsets` (`offsetsByGroup` entry absent +
no open txns) on the post-state of the regular sweep.
If we collapse the defer into `shouldExpire(config)`, the early return skips
`cleanupExpiredOffsets` for any streams group with
`storedDescriptionTopologyEpoch != -1`. That group's offsets never get
tombstoned, the topology cycle's eligibility never trips, the stored epoch
never gets cleared — and `shouldExpire(config)` stays false forever. The group
can't unstick itself.
A few refinements that preserve the abstraction without the dependency cycle:
1. Two methods on Group: keep `shouldExpire()` for the "skip entire sweep"
gate (ShareGroup's case), add a separate
`shouldTombstoneAfterOffsetExpiration(config)` (or similar name) for streams'
defer. The helper still goes away; the call site reads `if
(!group.shouldExpire()) return; … if
(!group.shouldTombstoneAfterOffsetExpiration(config)) return;`.
2. Reorder the sweep: always run `cleanupExpiredOffsets`, then check
`shouldExpire(config)` for the tombstone decision. Cleaner in one sense but
changes ShareGroup behaviour — its offsets would now be touched (no-op today
since share groups don't hold consumer-style offsets, but worth confirming).
3. Something else you had in mind? Maybe the topology cycle should drive its
own offset cleanup rather than depending on the regular sweep, which would
decouple them entirely — but that's a bigger restructure.
--
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]
