frankvicky commented on code in PR #22777:
URL: https://github.com/apache/kafka/pull/22777#discussion_r3570956578
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/StreamsGroupTopologyDescriptionManager.java:
##########
@@ -509,7 +532,8 @@ private CompletableFuture<Void> maybeAttachOne(
return null;
}
Integer storedEpoch =
result.storedDescriptionTopologyEpochs().get(describedGroup.groupId());
- if (storedEpoch == null || storedEpoch == -1 || storedEpoch !=
describedGroup.topology().epoch()) {
+ // <= NONE covers both NONE(-1) and UNCERTAIN(-2): nothing reliably
stored.
+ if (storedEpoch == null || storedEpoch <=
StreamsGroup.STORED_TOPOLOGY_EPOCH_NONE || storedEpoch !=
describedGroup.topology().epoch()) {
Review Comment:
This relies on the incidental ordering `NONE` = -1 > `UNCERTAIN` = -2 to
cover both sentinels in one comparison. It works today, but any future sentinel
introduced between them (or with a value outside this range) would silently
slip past the check and be treated as a real, reliably-stored epoch. The intent
— "neither value indicates a reliably stored topology" — is not visible from
the comparison alone; a reader has to know that every negative sentinel must be
excluded.
Suggest replacing with an explicit enumeration.
--
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]
