KAFKA-18185: remove internal.leave.group.on.close config #19400

frankvicky · 2025-04-07T07:40:55Z

This is a follow-up of #17614 The patch is to remove the
internal.leave.group.on.close config.

frankvicky · 2025-04-10T07:19:55Z

streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java

-                                            + " for the following reason: ",
-                                        exception.getCause()
-                                );
-                            }


IMHO, this chunk of code could be replaced with the new close api.

nice! love to see all this code get deleted for one line 😄

frankvicky · 2025-04-10T07:21:25Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

@@ -875,7 +878,7 @@ public void run() {
            streamsUncaughtExceptionHandler.accept(e, false);
            // Note: the above call currently rethrows the exception, so nothing below this line will be executed
        } finally {
-            completeShutdown(cleanRun);
+            completeShutdown(cleanRun, true);


If we close the StreamThread, we should ensure consumers leave their group.
Does it make sense?

ah sorry i totally missed this question on my first pass 🤦‍♀️ No, we actually do want to stay in the group and retain the existing default behavior for Streams

if you're curious why the default behavior for Streams is the opposite of the plain consumer client, it's because Streams is stateful which means rebalancing and moving partitions can be a much heavier operation and involve restoring many records from the changelog topics when partitions are shuffled around. By skipping the leave group, we can often avoid an unnecessary extra rebalance that would result in partitions being moved, since the assignment of Streams is heavily dependent on the current group members. If an error gets thrown and the StreamThread dies and gets restarted within the session timeout, we can avoid any rebalances where the dead thread is missing, so the rebalance when the new thread restarts should result in the same assignment of partitions to threads as we had before the thread died.

Thanks for the explanation!

frankvicky · 2025-04-10T07:23:27Z

tools/src/test/java/org/apache/kafka/tools/ResetIntegrationTest.java

        final boolean cleanResult = tryCleanGlobal(false, null, null, appID);
-        assertFalse(cleanResult);
+        assertTrue(cleanResult);


Regarding #19400 (comment)
This assertion is no longer false.
Make sense?

not 100% sure i follow, why does this change?

In https://github.com/apache/kafka/pull/19400/files#r2036678414
We have passed leaveGroup=true to the close method, which will make all consumers leave the group.
To keep the original behavior, we should pass leaveGroup=false to the close method.

I think we should maintain the original behavior for now; I will change it to false in the next commit.

oh i see, I missed that we were passing leaveGroup=true to the consumer before. Glad this test caught that, though maybe we should try to have a test specifically to make sure the default leave group behavior isn't ever changed by accident in the future?

frankvicky · 2025-04-10T07:24:46Z

streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java

@@ -1640,33 +1599,6 @@ public synchronized boolean close(final CloseOptions options) throws IllegalArgu
        return close(Optional.of(timeoutMs), options.leaveGroup);
    }

-    private Consumer<StreamThread> streamThreadLeaveConsumerGroup(final long remainingTimeMs) {


Same as https://github.com/apache/kafka/pull/19400/files#r2036675899

Hi @ableegoldman
Do you think we should keep this method?
Before KIP-1092, the consumer relied on admin#removeMembersFromConsumerGroup to leave the group.
As #19400 (comment) states, if we want to pass leaveGroup=false and maintain the default behavior, I think we should retain this method. Once a consumer is shut down and does not leave the group, they must rely on the admin to remove them from the consumer group.

This is also the reason that KafkaStreamsCloseOptionsIntegrationTest is failing.

this method is only ever called from KafkaStreams#close, there's no separate method KafkaStreams method that can be used to remove consumer from the group via the admin client (side note: maybe there should be?). There's a cli tool for static member removal so users who don't set leaveGroup=true in the CloseOptions can do so through this.

So the new consumer#close(CloseOptions.groupMembershipOperation(LEAVE_GROUP)) is essentially replacing this method entirely so in theory we should be able to remove it. That said, it's definitely concerning if KafkaStreamsCloseOptionsIntegrationTest is failing because we removed this. That suggests maybe our hack/workaround for removing static members in consumer#close via intentionally getting fenced doesn't work?

If that's the case then we should go back to using the admin's #removeMembersFromConsumerGroup for Streams for now, and can follow up with that KIP to do a proper fix for allowing static members to permanently leave via LeaveGroup.

frankvicky · 2025-04-10T07:27:00Z

Hi @ableegoldman
Please take a look when you have a free moment.
I have some questions since I'm not familiar with the stream module.
Please bear with me if the code looks like it was written while drunk.

ableegoldman

thanks for the followup!

ableegoldman · 2025-04-22T07:04:34Z

streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java

-                                            + " for the following reason: ",
-                                        exception.getCause()
-                                );
-                            }


nice! love to see all this code get deleted for one line 😄

ableegoldman · 2025-04-22T07:12:16Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

        } catch (final Throwable e) {
            log.error("Failed to close consumer due to the following error:", e);
        }
        try {
-            restoreConsumer.close();
+            restoreConsumer.close(closeOptions);


the restore consumer direct assignment so it's not part of the consumer group and doesn't need the close options (I don't remember, what happens if you pass in LEAVE_GROUP or REMAIN_IN_GROUP for a consumer that doesn't use group subscription?)

If the consumer doesn't subscribe, it will not join a group.
In this case, it will get an unknown member ID in the response.

I think we should pass either DEFAULT or REMAIN_IN_GROUP in for the restore consumer so it doesn't try to leave the group. If I'm remembering correctly, when LEAVE_GROUP is used it always tries to send a leave group even if it's not in a group, whereas with DEFAULT it will skip the leaveGroup if there's no member id or the coordinator is unknown

Hm actually how could it even send a LeaveGroup if LEAVE_GROUP is selected for a consumer that isn't part of a group? That is, by definition the coordinator is unknown, so where would the request even be sent?

Don't remember of the top of my head how we handled this in the consumer client CloseOptions PR but it seems like the consumer should be agnostic to the GroupMembershipOperation and never send anything if it's not part of a group to begin with, right?

ableegoldman · 2025-04-22T07:18:56Z

streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java

@@ -1994,6 +2005,13 @@ public Map<String, KafkaFuture<Uuid>> clientInstanceIds(final Duration timeout)
        return result;
    }

+    public void closeConsumer(final long timeoutMs, final boolean leaveGroup) {
+        final CloseOptions.GroupMembershipOperation operation = leaveGroup ? LEAVE_GROUP : REMAIN_IN_GROUP;
+        final CloseOptions closeOptions = CloseOptions.timeout(Duration.ofMillis(timeoutMs <= 0 ? 0 : timeoutMs))


it's definitely a bit questionable that we don't pass the timeout in to consumer#close in the current code (interestingly we also don't for producer#close but do use the timeout for admin#close) But, I don't want to change that in this PR, on the off chance there was an actual reason for it

I know we had some issues with the producer timeout on close in the past but I don't remember the specifics and I'm not personally aware of any similar issues for the consumer's close. It's worth following up on, but separately from this. Let's just use the default for now (ie don't set the timeout at all)

ableegoldman · 2025-04-22T07:21:11Z

tools/src/test/java/org/apache/kafka/tools/ResetIntegrationTest.java

        final boolean cleanResult = tryCleanGlobal(false, null, null, appID);
-        assertFalse(cleanResult);
+        assertTrue(cleanResult);


not 100% sure i follow, why does this change?

tools/src/test/java/org/apache/kafka/tools/ResetIntegrationTest.java

ableegoldman · 2025-04-24T23:48:12Z

looks like KafkaStreamsCloseOptionsIntegrationTest is failing

ableegoldman · 2025-04-26T01:48:22Z

@frankvicky this isn't strictly related to this PR but there's a KIP (1153) out to fix up the API for the Streams version of CloseOptions, would be great if you could take a look and give a +1 once it moves to voting

KAFKA-16758: remove internal.leave.group.on.close config

a4380ba

frankvicky added the ci-approved label Apr 7, 2025

github-actions bot added triage PRs from the community streams core Kafka Broker consumer connect clients small Small PRs labels Apr 7, 2025

frankvicky marked this pull request as draft April 7, 2025 09:53

frankvicky added 2 commits April 8, 2025 14:37

Merge branch 'trunk' into KAFKA-16758-follow-up

f482865

Adjust the logic of StreamThread#shutdown

f522eb1

frankvicky marked this pull request as ready for review April 8, 2025 07:45

github-actions bot removed the small Small PRs label Apr 8, 2025

frankvicky changed the title ~~KAFKA-16758: remove internal.leave.group.on.close config~~ KAFKA-18185: remove internal.leave.group.on.close config Apr 8, 2025

frankvicky added 5 commits April 8, 2025 16:03

Fix style issue

ed9f86d

Merge branch 'trunk' into KAFKA-16758-follow-up

9f47d9e

apply leaveGroup to StreamThread#shutdown

8a53219

Merge branch 'trunk' into KAFKA-16758-follow-up

dee54ad

Merge branch 'trunk' into KAFKA-16758-follow-up

00e013f

frankvicky marked this pull request as draft April 8, 2025 15:49

frankvicky added 2 commits April 9, 2025 12:18

Use close api instead of admin api

aef9785

Merge branch 'trunk' into KAFKA-16758-follow-up

d723718

frankvicky marked this pull request as ready for review April 9, 2025 04:21

frankvicky changed the title ~~KAFKA-18185: remove internal.leave.group.on.close config~~ [WIP] KAFKA-18185: remove internal.leave.group.on.close config Apr 9, 2025

frankvicky added 2 commits April 9, 2025 15:56

Use close api instead of admin api

6d95fcc

Merge branch 'trunk' into KAFKA-16758-follow-up

43df632

github-actions bot added the tools label Apr 9, 2025

frankvicky added 2 commits April 9, 2025 22:29

Merge branch 'trunk' into KAFKA-16758-follow-up

e5a4d96

fix failed tests

8649da6

frankvicky commented Apr 10, 2025

View reviewed changes

frankvicky changed the title ~~[WIP] KAFKA-18185: remove internal.leave.group.on.close config~~ KAFKA-18185: remove internal.leave.group.on.close config Apr 10, 2025

Merge branch 'trunk' into KAFKA-16758-follow-up

61efd8e

github-actions bot removed the triage PRs from the community label Apr 11, 2025

frankvicky added 8 commits April 12, 2025 19:57

Merge branch 'trunk' into KAFKA-16758-follow-up

2d5bec2

Merge branch 'trunk' into KAFKA-16758-follow-up

f5d1c72

Merge branch 'trunk' into KAFKA-16758-follow-up

1eed3f6

Merge branch 'trunk' into KAFKA-16758-follow-up

39c654e

Merge branch 'trunk' into KAFKA-16758-follow-up

cc80fb4

Merge branch 'trunk' into KAFKA-16758-follow-up

dc95556

Merge branch 'trunk' into KAFKA-16758-follow-up

62d8ed1

Merge branch 'trunk' into KAFKA-16758-follow-up

80f1771

ableegoldman reviewed Apr 22, 2025

View reviewed changes

frankvicky added 3 commits April 24, 2025 15:34

Merge branch 'trunk' into KAFKA-16758-follow-up

c356ae5

Remove timeout argument

38b3207

pass false to close method

0f04f13

ableegoldman reviewed Apr 24, 2025

View reviewed changes

tools/src/test/java/org/apache/kafka/tools/ResetIntegrationTest.java Outdated Show resolved Hide resolved

frankvicky added 3 commits April 25, 2025 12:26

Add comment back

2f9a230

address comments

d9e5eed

Merge branch 'trunk' into KAFKA-16758-follow-up

ffb74cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18185: remove internal.leave.group.on.close config #19400

KAFKA-18185: remove internal.leave.group.on.close config #19400

frankvicky commented Apr 7, 2025 •

edited by github-actions bot

Loading

frankvicky Apr 10, 2025

ableegoldman Apr 22, 2025

frankvicky Apr 10, 2025

ableegoldman Apr 24, 2025 •

edited

Loading

frankvicky Apr 25, 2025

frankvicky Apr 10, 2025

ableegoldman Apr 22, 2025

frankvicky Apr 24, 2025

ableegoldman Apr 24, 2025

frankvicky Apr 10, 2025

frankvicky Apr 25, 2025

frankvicky Apr 25, 2025

ableegoldman Apr 25, 2025

frankvicky commented Apr 10, 2025

ableegoldman left a comment

ableegoldman Apr 22, 2025

ableegoldman Apr 22, 2025

frankvicky Apr 24, 2025

ableegoldman Apr 24, 2025

ableegoldman Apr 24, 2025

ableegoldman Apr 22, 2025

ableegoldman Apr 22, 2025

ableegoldman commented Apr 24, 2025

ableegoldman commented Apr 26, 2025

KAFKA-18185: remove internal.leave.group.on.close config #19400

Are you sure you want to change the base?

KAFKA-18185: remove internal.leave.group.on.close config #19400

Conversation

frankvicky commented Apr 7, 2025 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ableegoldman Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankvicky commented Apr 10, 2025

ableegoldman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ableegoldman commented Apr 24, 2025

ableegoldman commented Apr 26, 2025

frankvicky commented Apr 7, 2025 •

edited by github-actions bot

Loading

ableegoldman Apr 24, 2025 •

edited

Loading