Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-14367; Add SyncGroup to the new GroupCoordinator interface #12847

Merged
merged 3 commits into from Dec 2, 2022

Conversation

dajac
Copy link
Contributor

@dajac dajac commented Nov 14, 2022

This patch adds syncGroup to the new GroupCoordinator interface and updates KafkaApis to use it.

Committer Checklist (excluded from commit message)

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

@dajac dajac added the KIP-848 label Nov 14, 2022
}

@Test
def testHandleSyncGroupRequestFutureFailed(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming my understanding, we added testHandleSyncGroupRequestFutureFailed and testHandleSyncGroupRequestAuthenticationFailed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right.

Copy link
Contributor

@jeffkbkim jeffkbkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i noticed that

org.apache.kafka.streams.integration.RestoreIntegrationTest

failed and the error message seems to indicate a rebalancing issue

java.lang.AssertionError: Expected all streams instances in [org.apache.kafka.streams.KafkaStreams@5e5d40ec] to be REBALANCING within 60000 ms, but the following were not: {org.apache.kafka.streams.KafkaStreams@5e5d40ec=RUNNING}

@dajac dajac changed the base branch from KAFKA-14367-join-group to trunk November 29, 2022 20:00
Copy link
Contributor

@jolshan jolshan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm -- test failures seemed unrelated

Copy link
Contributor

@jeffkbkim jeffkbkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a small comment

core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved
@showuon
Copy link
Contributor

showuon commented Dec 1, 2022

Sorry, was busy recently. I'll review it this week or next week.

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one minor naming suggestion.

@dajac dajac merged commit fd05073 into apache:trunk Dec 2, 2022
@dajac dajac deleted the KAFKA-14367-sync-group branch December 2, 2022 16:15
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
…pache#12847)

This patch adds `syncGroup` to the new `GroupCoordinator` interface and updates `KafkaApis` to use it.

Reviewers: Justine Olshan <jolshan@confluent.io>, Jeff Kim <jeff.kim@confluent.io>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants