KAFKA-9866: Avoid election for topics where preferred leader is not in ISR by leonardge · Pull Request #8524 · apache/kafka

leonardge · 2020-04-21T08:48:53Z

In this commit we made sure that the auto leader election only happens after the newly starter broker is in the isr.

No accompany tests are added due to the fact that:

this is a change to the private method and no public facing change is made
it is hard to create tests for this change without considerable effort

Committer Checklist (excluded from commit message)

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

…ync.

stanislavkozlovski

Thanks for the PR! Can we add a test?

stanislavkozlovski · 2020-04-22T13:12:45Z

After discussing online, we figured there isn't an easy way to test this scenario. There's significant work to be done to make KafkaController unit test-able

Good catch on the testLeaderAndIsrWhenEntireIsrOfflineAndUncleanLeaderElectionEnabled!

I think this change looks good. Let's wait for @hachikuji or @junrao to take a look.

junrao

@leonardge : Thanks for the PR. Just one comment below.

junrao · 2020-04-23T01:08:55Z

          !topicDeletionManager.isTopicQueuedUpForDeletion(tp.topic) &&
-          controllerContext.allTopics.contains(tp.topic))
+          controllerContext.allTopics.contains(tp.topic) &&
+          controllerContext.partitionLeadershipInfo.get(tp).forall(l => l.leaderAndIsr.isr.contains(leaderBroker))


The preferred leader election also checks for live brokers. So, perhaps we could just call PartitionLeaderElectionAlgorithms.preferredReplicaPartitionLeaderElection() here.

junrao

@leonardge : Thanks for the updated PR. One more followup comment.

junrao · 2020-04-23T18:51:40Z

+          PartitionLeaderElectionAlgorithms.preferredReplicaPartitionLeaderElection(
+            controllerContext.partitionReplicaAssignment(tp),
+            controllerContext.partitionLeadershipInfo(tp).leaderAndIsr.isr,
+            controllerContext.liveBrokerIds.toSet).nonEmpty


In Election.leaderForPreferredReplica(), liveReplicas is computed as the following. So, we probably want to be consistent here.

val liveReplicas = assignment.filter(replica => controllerContext.isReplicaOnline(replica, partition))

Done! And after using the mentioned implementation the code block gets cluttered so I extracted it into a helper method.

junrao

@leonardge : Thanks for the PR. Just a minor comment below.

junrao · 2020-04-24T17:24:17Z

    }
  }

+  private def isPreferredLeaderInSync(tp: TopicPartition): Boolean = {


Perhaps a more accurate name is canPreferredReplicaBeLeader()?

junrao · 2020-04-27T16:59:42Z

ok to test

junrao

@leonardge : Thanks for the latest update. LGTM. Waiting for the tests to pass.

ijuma · 2020-04-27T21:11:09Z


+  private def canPreferredReplicaBeLeader(tp: TopicPartition): Boolean = {
+    val assignment = controllerContext.partitionReplicaAssignment(tp)
+    val liveReplicas = assignment.filter(replica => controllerContext.isReplicaOnline(replica, tp))


We also do controllerContext.isReplicaOnline(leaderBroker, tp) in the caller. Do we also need it here?

Good point. We can get rid of the isReplicaOnline() check in the caller. @leonardge : Could you summit a followup minor PR?

Sure thing!

This is a minor follower up PR of #8524 Reviewer: Jun Rao <junrao@gmail.com>

…t-for-generated-requests * apache-github/trunk: (366 commits) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) KAFKA-9885; Evict last members of a group when the maximum allowed is reached (apache#8525) KAFKA-9866: Avoid election for topics where preferred leader is not in ISR (apache#8524) KAFKA-9839; Broker should accept control requests with newer broker epoch (apache#8509) KAKFA-9612: Add an option to kafka-configs.sh to add configs from a prop file (KIP-574) MINOR: Partition is under reassignment when adding and removing (apache#8364) MINOR: reduce allocations in log start and recovery checkpoints (apache#8467) MINOR: Remove unused foreign-key join class (apache#8547) HOTFIX: Fix broker bounce system tests (apache#8532) KAFKA-9704: Fix the issue z/OS won't let us resize file when mmap. (apache#8224) KAFKA-8639: Replace AddPartitionsToTxn with Automated Protocol (apache#8326) MINOR: equals() should compare all fields for generated classes (apache#8539) KAFKA-9844; Fix race condition which allows more than maximum number of members(apache#8454) KAFKA-9823: Remember the sent generation for the coordinator request (apache#8445) KAFKA-9883: Add better error message when REST API forwards a request and leader is not known (apache#8536) KAFKA-9907: Switch default build to Scala 2.13 (apache#8537) MINOR: Some html fixes in Streams DSL documentation (apache#8503) MINOR: Enable fatal warnings with scala 2.13 (apache#8429) KAFKA-9852: Change the max duration that calls to the buffer pool can block from 2000ms to 10ms to reduce overall test runtime (apache#8464) ...

Avoid starting election for topics where preferred leader is not in s…

44eea04

…ync.

leonardge changed the title ~~Avoid starting election for topics where preferred leader is not in s…~~ KAFKA-9866: Avoid starting election for topics where preferred leader is not in s… Apr 21, 2020

leonardge changed the title ~~KAFKA-9866: Avoid starting election for topics where preferred leader is not in s…~~ KAFKA-9866: Avoid starting election for topics where preferred leader is not in ISR Apr 21, 2020

leonardge changed the title ~~KAFKA-9866: Avoid starting election for topics where preferred leader is not in ISR~~ KAFKA-9866: Avoid election for topics where preferred leader is not in ISR Apr 21, 2020

stanislavkozlovski reviewed Apr 21, 2020

View reviewed changes

Comment thread core/src/main/scala/kafka/controller/KafkaController.scala Outdated

Refactored the code and fixed bug in the integration test.

29632f7

junrao reviewed Apr 23, 2020

View reviewed changes

Refactored according to comments.

172960f

junrao reviewed Apr 23, 2020

View reviewed changes

leonardge added 2 commits April 24, 2020 10:08

Kept the code to be consistent.

24c6d42

Reverted style change.

dc67b99

junrao reviewed Apr 24, 2020

View reviewed changes

Renamed helper function after review.

2fa3cee

leonardge requested a review from junrao April 25, 2020 23:46

junrao approved these changes Apr 27, 2020

View reviewed changes

junrao merged commit db9e55a into apache:trunk Apr 27, 2020

ijuma reviewed Apr 27, 2020

View reviewed changes

leonardge mentioned this pull request Apr 27, 2020

Fix minor code issue #8566

Merged

3 tasks

junrao pushed a commit that referenced this pull request Apr 28, 2020

Minor: remove redundant check in auto preferred leader election (#8566)

77ac06f

This is a minor follower up PR of #8524 Reviewer: Jun Rao <junrao@gmail.com>

Conversation

leonardge commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

stanislavkozlovski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stanislavkozlovski commented Apr 22, 2020

Uh oh!

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

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

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

junrao commented Apr 27, 2020

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leonardge commented Apr 21, 2020 •

edited

Loading