KAFKA-13783; Remove reason prefixing in JoinGroupRequest and LeaveGroupRequest by dajac · Pull Request #11971 · apache/kafka

dajac · 2022-03-30T08:12:57Z

KIP-800 introduced a mechanism to pass a reason in the join group request and in the leaver group request. A default reason is used unless one is provided by the user. In this case, the custom reason is prefixed by the default one.

When we tried to used this in Kafka Streams, we noted a significant degradation of the performances, see #11873. It is not clear wether the prefixing is the root cause of the issue or not. To be on the safe side, I think that we should remove the prefixing. It does not bring much anyway as we are still able to distinguish a custom reason from the default one on the broker side.

This patch removes prefixing the user provided reasons. So if a the user provides a reason, the reason is used directly. If the reason is empty or null, the default reason is used.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

showuon · 2022-03-30T13:29:15Z

I'll take a look tomorrow. Thanks.

dajac · 2022-03-30T14:55:34Z

cc @cadonna I'd like to get this one in 3.2.

cadonna · 2022-03-30T14:57:46Z

@dajac Please go ahead!

lihaosky · 2022-03-30T18:22:01Z

        List<MemberIdentity> membersToRemove = new ArrayList<>();
        for (final MemberDescription member : members) {
+            MemberIdentity memberIdentity = new MemberIdentity()
+                .setReason(reason);


QQ: seems reason is also used in equals, is it ok to add this? Will this cause membersToRemove not found from somewhere?

Good question. Let me double check this. For the context, we were doing this before this patch as well.

This does not seem to be an issue. I have extended the unit test to ensure that the look up works as we expect.

nit: does .setReason() have to be in its own line?

jeffkbkim

Thanks for the PR! left a few comments

jeffkbkim · 2022-03-30T22:44:47Z

        List<MemberIdentity> membersToRemove = new ArrayList<>();
        for (final MemberDescription member : members) {
+            MemberIdentity memberIdentity = new MemberIdentity()
+                .setReason(reason);


nit: does .setReason() have to be in its own line?

jeffkbkim · 2022-03-30T23:54:20Z

+
+        // a first rebalance to get the assignment, we need two poll calls since we need two round trips to finish join / sync-group
+        consumer.poll(Duration.ZERO);
+        consumer.poll(Duration.ZERO);


the test passes without the second poll. the first poll finishes the sync

INFO Successfully synced group in generation

before the second poll is triggered.
The second poll notifies the assignor and gets committed offsets which i don't think is necessary in this test

I think what we want is to wait for rebalance complete. we can explicitly wait like this:

TestUtils.waitForCondition(() -> { ConsumerRecords<String, String> recs = consumer.poll(Duration.ofMillis(100L)); return consumer.assignment().equals(Utils.mkSet(tp0, t2p0)); });

WDYT?

I just refactored this test based on @jeffkbkim's comment. In the end, we only care about the join group call in this context so I have changed the test to only care about this part.

jeffkbkim · 2022-03-30T23:56:42Z

+        String reason = options.reason() == null || options.reason().isEmpty() ?
+            DEFAULT_LEAVE_GROUP_REASON : options.reason();
+
        List<MemberIdentity> members;
        if (options.removeAll()) {
-            members = getMembersFromGroup(groupId);
+            members = getMembersFromGroup(groupId, reason);
        } else {
-            members = options.members().stream().map(MemberToRemove::toMemberIdentity).collect(Collectors.toList());
+            members = options.members().stream()
+                .map(m -> m.toMemberIdentity().setReason(reason))
+                .collect(Collectors.toList());


should this have been done as part of KIP-800?

This is a small refactoring of the code that we did in KIP-800. It basically set the reason when the member identity is created instead of re-iterating over the members afterwards. I think that it is a bit clearer this way. I guess that we could have done like this in the original implementation but I did not think about it last time.

showuon

Thanks for the PR. LGTM! Left a minor comment. Thanks.

showuon · 2022-03-31T06:18:48Z

+
+        // a first rebalance to get the assignment, we need two poll calls since we need two round trips to finish join / sync-group
+        consumer.poll(Duration.ZERO);
+        consumer.poll(Duration.ZERO);


I think what we want is to wait for rebalance complete. we can explicitly wait like this:

TestUtils.waitForCondition(() -> { ConsumerRecords<String, String> recs = consumer.poll(Duration.ofMillis(100L)); return consumer.assignment().equals(Utils.mkSet(tp0, t2p0)); });

WDYT?

dajac · 2022-03-31T06:56:35Z

@jeffkbkim @showuon Thanks for your comments. I have addressed them.

showuon

LGTM! Thanks.

…upRequest (#11971) KIP-800 introduced a mechanism to pass a reason in the join group request and in the leave group request. A default reason is used unless one is provided by the user. In this case, the custom reason is prefixed by the default one. When we tried to used this in Kafka Streams, we noted a significant degradation of the performances, see #11873. It is not clear wether the prefixing is the root cause of the issue or not. To be on the safe side, I think that we should remove the prefixing. It does not bring much anyway as we are still able to distinguish a custom reason from the default one on the broker side. This patch removes prefixing the user provided reasons. So if a the user provides a reason, the reason is used directly. If the reason is empty or null, the default reason is used. Reviewers: Luke Chen <showuon@gmail.com>, <jeff.kim@confluent.io>, Hao Li <hli@confluent.io>

dajac · 2022-03-31T12:34:41Z

Merged to trunk and to 3.2.

ijuma · 2022-03-31T12:53:40Z

Can we make sure this actually helps with the benchmark that previously regressed?

cadonna · 2022-03-31T12:57:08Z

@lihaosky Could you please try to re-add the leave reason in Streams and re-run the benchmarks?

lihaosky · 2022-03-31T16:25:07Z

Sure thing!

dajac · 2022-04-07T07:33:40Z

We have found the root cause of the original issue. It was a bug in our benchmark setup so it had nothing to do with this change in the end. We can still keep this change as it looks better to me like this.

showuon · 2022-04-07T08:47:47Z

We have found the root cause of the original issue. It was a bug in our benchmark setup so it had nothing to do with this change in the end. We can still keep this change as it looks better to me like this.

Thanks for the update. Great to hear that!

dajac added 2 commits March 30, 2022 09:31

Remove reason prefix in the admin client

92ebea1

Remove default reason prefix in consumer

e0ea05d

dajac requested a review from showuon March 30, 2022 08:20

lihaosky reviewed Mar 30, 2022

View reviewed changes

extend unit test

36b71f8

jeffkbkim reviewed Mar 30, 2022

View reviewed changes

showuon reviewed Mar 31, 2022

View reviewed changes

refactor and address comments

a6c6763

showuon approved these changes Mar 31, 2022

View reviewed changes

dajac merged commit ce7788a into apache:trunk Mar 31, 2022

dajac deleted the minor-remove-reason-prefix branch March 31, 2022 12:34

Conversation

dajac commented Mar 30, 2022

Committer Checklist (excluded from commit message)

Uh oh!

showuon commented Mar 30, 2022

Uh oh!

dajac commented Mar 30, 2022

Uh oh!

cadonna commented Mar 30, 2022

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!

jeffkbkim 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!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac commented Mar 31, 2022

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Mar 31, 2022

Uh oh!

ijuma commented Mar 31, 2022

Uh oh!

cadonna commented Mar 31, 2022

Uh oh!

lihaosky commented Mar 31, 2022

Uh oh!

dajac commented Apr 7, 2022

Uh oh!

showuon commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants