KAFKA-8286; Leader Election Admin RPC (KIP-460) by jsancio · Pull Request #6686 · apache/kafka

jsancio · 2019-05-06T22:54:28Z

Implements KIP-460: https://cwiki.apache.org/confluence/display/KAFKA/KIP-460%3A+Admin+Leader+Election+RPC

Pending:

Implement additional tests
Improve documentation

Committer Checklist (excluded from commit message)

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

Also remove any remaining references to "preferred".

jsancio · 2019-05-13T19:47:37Z

cc @hachikuji @mumrah @junrao

hachikuji

Thanks for the patch. Left a few initial comments.

jsancio

Thanks for the feedback. I'll address your comments today.

Regarding the server/controller side of the changes feel free to take a glance but hold off on a detail review. I found a major issue with the implementation that I have addressed and I am in the process of cleaning up.

cc @mumrah

hachikuji

Just a few more comments. The controller changes look pretty clean.

junrao

@jsancio : Thanks for the PR. Made a pass of all the server side files. A few comments below.

hachikuji

Did another pass over the client-side changes. Will look at the server stuff next.

hachikuji · 2019-05-17T01:35:18Z

In terms of error handling, I think these are the main cases:

Broker is not the controller => CONTROLLER_MOVED
Broker's zk write is fenced by another controller => Not 100% sure, but this raises a StateChangeFailedException which seems to get mapped to ELIGIBLE_LEADERS_NOT_AVAILABLE. Maybe this should be CONTROLLER_MOVED also?
Unclean/preferred election not needed => I was having trouble tracking this down, but I don't think we have a separate error code, so maybe it's ELIGIBLE_LEADERS_NOT_AVAILABLE? Should we have a new error code, maybe ELECTION_NOT_NEEDED?
Unclean/preferred election not possible => ELIGIBLE_LEADERS_NOT_AVAILBLE
Election succeeded => NONE

Just want to check high level if there are any additional cases to check for and if we agree from an API perspective on how they should be handled.

Do not assume that if the admin client receives an empty set of partitions when it did not provide any partitions that it means that we had a cluster authoriazation error.

jsancio · 2019-05-17T22:07:28Z

In terms of error handling, I think these are the main cases:

1. Broker is not the controller => CONTROLLER_MOVED

2. Broker's zk write is fenced by another controller => Not 100% sure, but this raises a StateChangeFailedException which seems to get mapped to ELIGIBLE_LEADERS_NOT_AVAILABLE. Maybe this should be CONTROLLER_MOVED also?

3. Unclean/preferred election not needed => I was having trouble tracking this down, but I don't think we have a separate error code, so maybe it's ELIGIBLE_LEADERS_NOT_AVAILABLE? Should we have a new error code, maybe ELECTION_NOT_NEEDED?

4. Unclean/preferred election not possible => ELIGIBLE_LEADERS_NOT_AVAILBLE

5. Election succeeded => NONE

Just want to check high level if there are any additional cases to check for and if we agree from an API perspective on how they should be handled.

I filled the following issues. I will be working on them right after this PR.

hachikuji

Thanks for the updates. Looking good overall. It would be nice to see some integration/system tests.

When the requesting elections for all of the topic partitions, do not return topic partitions that didn't need to change leader.

hachikuji

Thanks for the updates. Left some more comments.

jsancio · 2019-05-24T22:36:06Z

retest this please

ijuma · 2019-05-28T14:07:15Z

retest this please

jsancio · 2019-05-28T16:22:42Z

retest this please

hachikuji

Thanks for the patch! LGTM.

jsancio · 2019-05-29T14:49:30Z

JDK 8 and Scala 2.11 passes. There is one failure in JDK 11 and Scala 2.12

kafka.integration.UncleanLeaderElectionTest.testUncleanLeaderElectionDisabled

jsancio · 2019-05-29T14:49:42Z

retest this please

) Implements KIP-460: https://cwiki.apache.org/confluence/display/KAFKA/KIP-460%3A+Admin+Leader+Election+RPC. Reviewers: Jun Rao <junrao@gmail.com>, Jason Gustafson <jason@confluent.io>

jsancio added 7 commits May 6, 2019 15:33

Change the elect leader RPC

1144c98

Election type support in the controller

d6728de

Implement kafka-elect-leaders command

6a5a58b

Add PartitionLeaderNotAvailable exception

3261276

Also remove any remaining references to "preferred".

Add tests for partition state machine

cf9e4c8

Merge the preferred election command test suites

23f6eeb

Add framework for testing LeaderElectionCommand

33ec321

jsancio force-pushed the election-rpc branch from bdc1285 to 33ec321 Compare May 13, 2019 19:43

Close AdminClient after elect leader command

fcebc1e

hachikuji reviewed May 14, 2019

View reviewed changes

jsancio commented May 14, 2019

View reviewed changes

hachikuji reviewed May 14, 2019

View reviewed changes

Propagate leader information during election

5bcf3cf

hachikuji reviewed May 14, 2019

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/requests/ElectLeadersRequest.java Outdated

junrao reviewed May 15, 2019

View reviewed changes

Refactor controller to return leader election result

35af0cc

mumrah reviewed May 15, 2019

View reviewed changes

jsancio added 3 commits May 16, 2019 10:21

Add a top level error to the leader election RPC

0f945b3

Query all partitions after authorization

144d8c2

Only fetch topic config if allowUnclean is not set

41b16c9

hachikuji reviewed May 16, 2019

View reviewed changes

Implement electPreferredLeaders using electLeaders

9ddba69

hachikuji reviewed May 16, 2019

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java Outdated

Do not assume cluster authorization error

8674e72

Do not assume that if the admin client receives an empty set of partitions when it did not provide any partitions that it means that we had a cluster authoriazation error.

jsancio added 3 commits May 20, 2019 10:42

Change old command error semantic to mathc the new electLeaders RPC

282d900

Merge remote-tracking branch 'upstream/trunk' into election-rpc

e61f7ab

Update documentation

524e8b9

jsancio changed the title ~~[WIP] KIP-460 Leader Election Admin RPC~~ KIP-460 Leader Election Admin RPC May 21, 2019

jsancio marked this pull request as ready for review May 21, 2019 00:19

Change the admin test to now use deprecated APIs

242b413

hachikuji reviewed May 22, 2019

View reviewed changes

jsancio added 3 commits May 22, 2019 11:14

Don't return partitions that didn't need election

6ebb040

When the requesting elections for all of the topic partitions, do not return topic partitions that didn't need to change leader.

Move the byte to ElectionType conversion function

edbe381

Implement all() for ElectLeadersResult

48cfdfa

hachikuji reviewed May 23, 2019

View reviewed changes

jsancio added 2 commits May 23, 2019 15:36

Avoid unnecessary copy when generating response

9f3466d

Merge remote-tracking branch 'upstream/trunk' into election-rpc

fdc5dfb

hachikuji mentioned this pull request May 24, 2019

MINOR: trivial cleanups #6767

Merged

3 tasks

jsancio added 4 commits May 24, 2019 09:20

For unclean filter partitions with a live leader

b806b26

Use Set for collection of topics

4fd6420

Merge remote-tracking branch 'upstream/trunk' into election-rpc

092b3e4

It compiles in scala 2.11.x

7dd3e01

Merge remote-tracking branch 'upstream/trunk' into election-rpc

0709771

hachikuji changed the title ~~KIP-460 Leader Election Admin RPC~~ KAFKA-8286; Leader Election Admin RPC (KIP-460) May 28, 2019

hachikuji approved these changes May 28, 2019

View reviewed changes

jsancio added 2 commits May 28, 2019 12:32

Remove extra match case introduced during merge

95432ce

Make a mutable copy of the topic Set

6d53f26

hachikuji merged commit 121308c into apache:trunk May 29, 2019

jsancio deleted the election-rpc branch May 29, 2019 18:57

big-andy-coates mentioned this pull request May 30, 2019

Fixing the build. (Backing out KIP-460 changes until they come through) confluentinc/ksql#2900

Merged

2 tasks

Uh oh!

Conversation

jsancio commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

jsancio commented May 13, 2019

Uh oh!

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

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Uh oh!

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

hachikuji commented May 17, 2019

Uh oh!

jsancio commented May 17, 2019

Uh oh!

jsancio commented May 6, 2019 •

edited

Loading