KAFKA-21102: Soften the validation for group upgrade and downgrade paths by suzhiking · Pull Request #23530 · apache/kafka

suzhiking · 2026-09-21T01:54:31Z

Soften tombstone replay exception to log for consumer/streams/share
group. Also add missing tests for consumer/share group.

Reviewers: rishi-rana rishi-rana@hotmail.com, Sean Quah squah@confluent.io

…e group

rishi-rana · 2026-09-22T00:49:36Z

+                    memberId, String.join(" and ", reasons));
            }
+
            consumerGroup.removeMember(memberId);


When this condition (line 5930: targetAssignment().containsKey(memberId)) is hit, this now warns and calls removeMember(memberId) — but ConsumerGroup#removeMember doesn't touch targetAssignment (same for ShareGroup#removeMember and StreamsGroup#removeMember), and nothing else will clean up that entry for a member that's already gone. So this leaves a permanently orphaned entry in targetAssignment(), not just a logged compaction-artifact warning.

Should this also call removeTargetAssignment(memberId) (already available on the shared ModernGroup base class) here? Same question applies to the two sibling occurrences: share group (line 6352 condition / line 6365 removeMember call) and streams group (line 6459 condition / line 6472 removeMember call).

I've thought about this but I was not sure what's impact of clearing target assignments so just left it as it.

updated PR with cleanup

I wonder if we should invoke replay with synthetic tombstones when we detect missing ones, instead of duplicating the tombstone logic from other methods. It could be more robust against future changes. In normal operation we will almost never take the missing tombstone path so we don't need to worry about performance that much.

agree. I will update so that every path that has missing sibling tombstones will replay these tombstones

squah-confluent

Thanks for the patch and for checking all three group types!

squah-confluent · 2026-09-24T08:04:31Z

+            if (oldMember.memberEpoch() != LEAVE_GROUP_MEMBER_EPOCH || consumerGroup.targetAssignment().containsKey(memberId)) {
+                List<String> reasons = new ArrayList<>();
+                if (oldMember.memberEpoch() != LEAVE_GROUP_MEMBER_EPOCH) {
+                    reasons.add("has invalid leave group epoch");


The tombstone that's missing is a ConsumerGroupCurrentMemberAssignment. How about wording it like this?

Suggested change

reasons.add("has invalid leave group epoch");

reasons.add("still has a current assignment");

The same for share and streams groups.

squah-confluent · 2026-09-24T08:04:59Z

+                    reasons.add("has invalid leave group epoch");
+                }
+                if (consumerGroup.targetAssignment().containsKey(memberId)) {
+                    reasons.add("member exists in target assignment");


typo? when formatted we will render "but member member exists in target assignment".

Suggested change

reasons.add("member exists in target assignment");

reasons.add("still has a target assignment");

The same for share and streams groups.

yea mb, copy-pasted the sentence

squah-confluent · 2026-09-24T08:06:19Z

+                    memberId, String.join(" and ", reasons));
            }
+
            consumerGroup.removeMember(memberId);


I wonder if we should invoke replay with synthetic tombstones when we detect missing ones, instead of duplicating the tombstone logic from other methods. It could be more robust against future changes. In normal operation we will almost never take the missing tombstone path so we don't need to worry about performance that much.

squah-confluent · 2026-09-24T08:06:47Z

+                    reasons.add(consumerGroup.targetAssignment().size() + " target assignments");
+                }
+                if (consumerGroup.assignmentEpoch() != -1) {
+                    reasons.add("assignment epoch " + consumerGroup.assignmentEpoch());


Suggested change

reasons.add("assignment epoch " + consumerGroup.assignmentEpoch());

reasons.add("target assignment epoch " + consumerGroup.assignmentEpoch());

The same for share and streams groups.

squah-confluent · 2026-09-24T09:29:56Z

+        // The sibling tombstones arriving afterwards are no-ops.
+        context.replay(GroupCoordinatorRecordHelpers.newConsumerGroupCurrentAssignmentTombstoneRecord("foo", "m1"));
+        context.replay(GroupCoordinatorRecordHelpers.newConsumerGroupTargetAssignmentTombstoneRecord("foo", "m1"));
+
+        assertFalse(context.groupMetadataManager.consumerGroup("foo").hasMember("m1"));
+        assertFalse(context.groupMetadataManager.consumerGroup("foo").targetAssignment().containsKey("m1"));


Aren't these already tested by the sibling tombstone test methods?

(may apply elsewhere too)

you are right, it's true for consumer and streams, but shares group's existing tests do not test this. I'll add missing tests for shared group

squah-confluent · 2026-09-24T09:47:20Z

+    }
+
+    @Test
+    public void testReplayConsumerGroupEpochTombstoneExisting() {


The record type is called ConsumerGroupMetadata, so this method should start with testReplayConsumerGroupMetadataTombstone.

(may apply elsewhere too)

squah-confluent · 2026-09-24T09:48:31Z

    }

+    @Test
+    public void testReplayConsumerGroupMemberMetadataTombstoneExisting() {


The existing testReplay methods are grouped by record type. Can we preserve the grouping?

squah-confluent · 2026-09-24T10:06:39Z

    }

+    @Test
+    public void testReplayConsumerGroupMemberMetadataTombstoneExisting() {


There's already a testReplayConsumerGroupMemberMetadataTombstone method that tombstones an existing group. Can we choose a suffix that means "existing, but other tombstones are missing"?

agree, changed suffix to WithMissingSiblingTombstones

When a member, group, or target-assignment-metadata tombstone is replayed while in-memory state shows that sibling tombstones written in the same deletion were never seen (a compaction artifact), replay those missing tombstones through their own replay methods instead of duplicating their effects inline. A member tombstone replays the member's current-assignment and target-assignment tombstones; a group tombstone replays the remaining member tombstones, leftover target-assignment tombstones and the target-assignment-metadata tombstone; a target-assignment-metadata tombstone replays the leftover target-assignment tombstones. This removes the hand-rolled target-assignment and topic-subscription cleanup and keeps the repair in sync with whatever each tombstone replay does. Each replay logs a single warning per received tombstone listing the inconsistencies that were repaired.

Name the tests that replay a tombstone while sibling tombstones are missing with a WithMissingSiblingTombstones suffix, since the Existing suffix already means "the group exists" in this file. Use the record type name (ConsumerGroupMetadata rather than Epoch). Place each test next to the other tests for its record type. Drop the trailing replays of sibling tombstones, which only exercised the no-op paths already covered by the dedicated tombstone tests.

…stone The target-assignment-metadata tombstone is only ever written as part of a group deletion, after the per-member target-assignment tombstones, so any per-member entries still present when it is replayed are an artifact and are now removed by replaying their tombstones.

Mirror the existing consumer group tests that replay a tombstone for a missing member or a missing group and assert it is ignored, which share groups lacked.

Softening tombstone replay exception to log for consumer/streams/shar…

7850f1f

…e group

github-actions Bot added triage PRs from the community group-coordinator labels Sep 21, 2026

squah-confluent self-requested a review September 21, 2026 15:13

dajac self-requested a review September 21, 2026 16:23

dajac removed the triage PRs from the community label Sep 21, 2026

rishi-rana reviewed Sep 22, 2026

View reviewed changes

Remove the member's target assignment when replaying its tombstone

3cba21b

squah-confluent reviewed Sep 24, 2026

View reviewed changes

suzhiking added 4 commits September 24, 2026 17:03

Add share group no-op tombstone replay tests

f76ecd8

Mirror the existing consumer group tests that replay a tombstone for a missing member or a missing group and assert it is ignored, which share groups lacked.

	reasons.add("has invalid leave group epoch");
	reasons.add("still has a current assignment");

	reasons.add("member exists in target assignment");
	reasons.add("still has a target assignment");

	reasons.add("assignment epoch " + consumerGroup.assignmentEpoch());
	reasons.add("target assignment epoch " + consumerGroup.assignmentEpoch());

Conversation

suzhiking commented Sep 21, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suzhiking Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squah-confluent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squah-confluent Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

suzhiking commented Sep 21, 2026 •

edited by github-actions Bot

Loading

suzhiking Sep 22, 2026 •

edited

Loading

squah-confluent Sep 24, 2026 •

edited

Loading