lianetm commented on code in PR #19744:
URL: https://github.com/apache/kafka/pull/19744#discussion_r2107800303
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/OffsetMetadataManager.java:
##########
@@ -859,6 +859,7 @@ boolean hasCommittedOffset(
*
* @return A List of OffsetFetchResponseTopics response.
*/
+ @SuppressWarnings("NPathComplexity")
Review Comment:
Can we maybe avoid the suppression if we use a var for the new check?
(something like `var isInvalidOffset = offsetAndMetadata == null ||
isMismatchedTopicId(...` and check on it on ln 910?
##########
core/src/test/scala/unit/kafka/server/OffsetFetchRequestTest.scala:
##########
@@ -527,4 +527,88 @@ class OffsetFetchRequestTest(cluster: ClusterInstance)
extends GroupCoordinatorB
)
}
}
+
+ @ClusterTest
+ def testFetchOffsetWithRecreatedTopic(): Unit = {
+ // There are two ways to ensure that committed of recreated topics are not
returned.
+ // 1) When a topic is deleted, GroupCoordinatorService#onPartitionsDeleted
is called to
+ // delete all its committed offsets.
+ // 2) Since version 10 of the OffsetCommit API, the topic id is stored
alongside the
+ // committed offset. When it is queried, it is only returned iff the
topic id of
+ // committed offset matches the requested one.
+ // The test tests both conditions but not in a deterministic way as they
race
+ // against each others.
Review Comment:
yeap, tricky to play against that here on integration tests, but seems good
enough because it ensures the final outcome (combined with the unit tests for
the topic ID check)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
