KAFKA-14500; [5/N] Implement JoinGroup protocol in new GroupCoordinator by jeffkbkim · Pull Request #13870 · apache/kafka

jeffkbkim · 2023-06-16T22:00:26Z

Built on top of #13812, this PR aims to implement the existing JoinGroup protocol within the new group coordinator.

For reviewing, one can start with GroupMetadataManager#genericGroupJoin().

Some notable differences:

methods return a CoordinatorResult to the runtime framework, which includes records to append to the log as well as a future to complete after the append succeeds/fails.
The coordinator runtime ensures that only a single thread will be processing a group at any given time, therefore there is no more locking on groups.
Instead of using on purgatories, we rely on the Timer interface to schedule/cancel delayed operations.

Committer Checklist (excluded from commit message)

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

dajac

@jeffkbkim Thanks for the PR. I have started to read it. I left a few preliminary comments.

dajac

I am still processing the PR. I left a few more comments in the meantime.

dajac

I am still processing the PR. I left a few more comments.

dajac

@jeffkbkim Thanks for the hard work on this one! I have compared the new implementation with the old implementation line by line. It looks pretty good. I left some comments, mainly nits. I will do a second pass on it on Monday and start looking into the tests as well.

dajac · 2023-07-07T14:15:01Z

Why do we need a copy here?

in GroupMetadataManager#expirePendingSync() we remove members from the set while iterating

Would it make sense to make the copy on the other side? It is a bit weird to anticipate this here because we don't do this for other accessors. I am usually tempted to return unmodifiable collections in the case to prevent this kind of issue.

This was not addressed.

thought i addressed this. addressed it now

dajac

@jeffkbkim Thanks for the hard work on this one! I have compared the new implementation with the old implementation line by line. It looks pretty good. I left some comments, mainly nits. I will do a second pass on it on Monday and start looking into the tests as well.

dajac · 2023-07-07T14:21:16Z

There are issues with the build as well. Could you look into this?

jeffkbkim · 2023-07-07T17:57:38Z

@dajac @CalvinConfluent

thanks for the review, i have addressed your comments.

dajac

I just made a pass on the main files and left more comments/suggestions.

dajac · 2023-07-10T12:59:31Z

I wonder if we should remove this because it will log all errors now.

this would log all errors while appending/committing and if generateRecordsAndResponse throws an unexpected exception. shouldn't we log them?

it doesn't seem like we do for consumerGroupHeartbeat() -- maybe just filter out the coordinator not available / not coordinator error codes?

Yeah, I think that it depends on what we mean by unexpected. I would remove it for now given that we also log something when the append future fails. We can always bring it back later if needed.

This was not addressed.

i was thinking about the illegal state exceptions. wouldn't we hide the issue then? maybe we can log only for non api exceptions. wdyt?

changed to logging only when the response future is not complete

dajac

I am half way through the tests. Continuing...

dajac

I left a few more comments related to the remaining tests.

jeffkbkim · 2023-07-12T02:46:42Z

@dajac thanks for the review. I have addressed your comments.

dajac · 2023-07-12T09:13:11Z

@jeffkbkim Now that #13963 is merged, could you update your PR?

jeffkbkim · 2023-07-12T19:58:50Z

@dajac I have updated with latest and unified the MockCoordinatorTimer.

dajac

@jeffkbkim Thanks for the update. I just made a full pass on it and I left some small comments (mainly nits). We should be good to merge it afterwards.

dajac · 2023-07-14T07:56:38Z

Yeah, I think that it depends on what we mean by unexpected. I would remove it for now given that we also log something when the append future fails. We can always bring it back later if needed.

checkstyle remove unused address comments address comments; add replay generate record when creating new group use scheduleWriteOperation generate records for new group only when no records were created address comments address comments, fix tests build fix address comments build fix small fix address comments address comments validate records on caller's side separate test for new group records fix build

jeffkbkim · 2023-07-19T02:47:59Z

the test failures are unrelated

dajac

LGTM. Thanks @jeffkbkim!

…or (apache#13870) This patch implements the existing JoinGroup protocol within the new group coordinator. Some notable differences: * Methods return a CoordinatorResult to the runtime framework, which includes records to append to the log as well as a future to complete after the append succeeds/fails. * The coordinator runtime ensures that only a single thread will be processing a group at any given time, therefore there is no more locking on groups. * Instead of using on purgatories, we rely on the Timer interface to schedule/cancel delayed operations. Reviewers: David Jacot <djacot@confluent.io>

jeffkbkim changed the title ~~KAFKA-14500; [5/N]~~ KAFKA-14500; [5/N] Implement JoinGroup protocol in new GroupCoordinator Jun 16, 2023

dajac added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Jun 17, 2023

dajac self-requested a review June 17, 2023 02:25

dajac reviewed Jun 20, 2023

View reviewed changes

jeffkbkim force-pushed the KAFKA-14500-5 branch from 3048771 to 9356dc8 Compare June 22, 2023 14:43

dajac reviewed Jun 22, 2023

View reviewed changes

Comment thread ...coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java Outdated

dajac reviewed Jun 22, 2023

View reviewed changes

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java Outdated

CalvinLiu7947 reviewed Jun 29, 2023

View reviewed changes

dajac reviewed Jun 30, 2023

View reviewed changes

jeffkbkim force-pushed the KAFKA-14500-5 branch from 73ed1e6 to f6ae13a Compare July 6, 2023 04:44

jeffkbkim mentioned this pull request Jul 7, 2023

KAFKA-14462; [21/N] Add CoordinatorTimer implementation in CoordinatorRuntime #13961

Merged

3 tasks

dajac reviewed Jul 7, 2023

View reviewed changes

dajac reviewed Jul 10, 2023

View reviewed changes

jeffkbkim force-pushed the KAFKA-14500-5 branch from dc47683 to d4dd3f4 Compare July 10, 2023 18:06

dajac reviewed Jul 11, 2023

View reviewed changes

jeffkbkim force-pushed the KAFKA-14500-5 branch from 5e731a2 to 332b261 Compare July 12, 2023 17:34

dajac reviewed Jul 14, 2023

View reviewed changes

jeffkbkim mentioned this pull request Jul 14, 2023

KAFKA-14500; [6/6] Implement SyncGroup protocol in new GroupCoordinator #14017

Merged

3 tasks

dajac reviewed Jul 17, 2023

View reviewed changes

Comment thread ...p-coordinator/src/test/java/org/apache/kafka/coordinator/group/GroupMetadataManagerTest.java Outdated

jeffkbkim added 3 commits July 17, 2023 15:20

unify MockCoordinatorTimer

9dbb770

tidy

2f55300

jeffkbkim added 5 commits July 17, 2023 15:20

checkstyle

dc6979e

address comments

23209ce

address comments

c4c74cb

checkstyle

bf2ea19

log only on incomplete response futures

2797b9b

dajac reviewed Jul 17, 2023

View reviewed changes

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java Outdated

build fix

a9bb3c3

jeffkbkim force-pushed the KAFKA-14500-5 branch from 13e704c to a9bb3c3 Compare July 17, 2023 19:34

only log when not kafka exception

4de7890

jeffkbkim commented Jul 18, 2023

View reviewed changes

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupMetadataManager.java Outdated

dajac reviewed Jul 18, 2023

View reviewed changes

Comment thread core/src/main/scala/kafka/server/BrokerServer.scala Outdated

dajac reviewed Jul 18, 2023

View reviewed changes

Comment thread group-coordinator/src/main/java/org/apache/kafka/coordinator/group/GroupCoordinatorService.java Outdated

jeffkbkim added 4 commits July 18, 2023 11:24

address comments

17459f4

fix test

45c99f6

Trigger build

7b82894

fix test

1d21547

dajac approved these changes Jul 19, 2023

View reviewed changes

dajac merged commit a500c3e into apache:trunk Jul 19, 2023

Uh oh!

Conversation

jeffkbkim commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Jul 7, 2023

Uh oh!

jeffkbkim commented Jul 7, 2023

Uh oh!

jeffkbkim commented Jun 16, 2023 •

edited

Loading