KAFKA-14296; Partition leaders are not demoted during kraft controlled shutdown by dajac · Pull Request #12741 · apache/kafka

dajac · 2022-10-13T08:02:50Z

When the BrokerServer starts its shutting down process, it transitions to SHUTTING_DOWN and sets isShuttingDown to true. With this state change, the follower state changes are short-cutted. This means that a broker which was serving as leader would remain acting as a leader until controlled shutdown completes. Instead, we want the leader and ISR state to be updated so that requests will return NOT_LEADER and the client can find the new leader.

We missed this case while implementing #12187.

This patch fixes the issue and updates an existing test to ensure that isShuttingDown has not effect. We should consider adding integration tests for this as well. We can do this separately.

Committer Checklist (excluded from commit message)

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

…d shutdown

ijuma · 2022-10-13T13:28:41Z

core/src/main/scala/kafka/server/ReplicaManager.scala

  ): Unit = {
    stateChangeLogger.info(s"Transitioning ${localFollowers.size} partition(s) to " +
      "local followers.")
-    val shuttingDown = isShuttingDown.get()


This code is a bit brittle, can we have something like the following as the field instead?

private Supplier brokerState = null;

Then we don't have to make sure to update isShuttingDown when the logic changes in KafkaServer.

I suppose that you are saying that relying on isShuttingDown is brittle, right? If that is the case, I do agree that relying on the broker state is better. I think that we can refactor the remaining usages as a follow-up.

ijuma · 2022-10-13T13:30:03Z

core/src/main/scala/kafka/server/ReplicaManager.scala

-            val state = info.partition.toLeaderAndIsrPartitionState(tp, isNew)
-            val isNewLeaderEpoch = partition.makeFollower(state, offsetCheckpoints, Some(info.topicId))
-
-            if (isInControlledShutdown && (info.partition.leader == NO_LEADER ||


Similarly, why do we need this special isInControlledShutdown field versus relying on the broker state as I mentioned above? This kind of approach is extremely brittle in my opinion.

We want to do this only when we are in controlled shutdown and it could be disabled. If you look at the caller side, we aligned this on how we do it for the lifecycleManager as both go together. We could revise this though.

hachikuji · 2022-10-13T16:47:01Z

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala

    mockReplicaFetcherManager: Option[ReplicaFetcherManager] = None,
-    mockReplicaAlterLogDirsManager: Option[ReplicaAlterLogDirsManager] = None
+    mockReplicaAlterLogDirsManager: Option[ReplicaAlterLogDirsManager] = None,
+    isShuttingDown: AtomicBoolean = new AtomicBoolean(false)


I found the use of the field a bit confusing. We don't necessarily have to fix it here, but what do you think about letting ReplicaManager own its own shutdown? Basically create a private field which serves the same purpose and then expose a method to explicitly toggle it. It is not obvious when looking at BrokerServer that altering the local field has this remote effect.

hachikuji

Thanks, LGTM

jsancio

LGTM. Thanks for the changes.

dajac · 2022-10-13T18:01:19Z

Will merge it tomorrow to trunk and 3.3. I am waiting on getting a clean build.

…d shutdown (#12741) When the `BrokerServer` starts its shutting down process, it transitions to `SHUTTING_DOWN` and sets `isShuttingDown` to `true`. With this state change, the follower state changes are short-cutted. This means that a broker which was serving as leader would remain acting as a leader until controlled shutdown completes. Instead, we want the leader and ISR state to be updated so that requests will return NOT_LEADER and the client can find the new leader. We missed this case while implementing #12187. This patch fixes the issue and updates an existing test to ensure that `isShuttingDown` has not effect. We should consider adding integration tests for this as well. We can do this separately. Reviewers: Ismael Juma <ismael@juma.me.uk>, José Armando García Sancio <jsancio@users.noreply.github.com>, Jason Gustafson <jason@confluent.io>

…d shutdown (apache#12741) When the `BrokerServer` starts its shutting down process, it transitions to `SHUTTING_DOWN` and sets `isShuttingDown` to `true`. With this state change, the follower state changes are short-cutted. This means that a broker which was serving as leader would remain acting as a leader until controlled shutdown completes. Instead, we want the leader and ISR state to be updated so that requests will return NOT_LEADER and the client can find the new leader. We missed this case while implementing apache#12187. This patch fixes the issue and updates an existing test to ensure that `isShuttingDown` has not effect. We should consider adding integration tests for this as well. We can do this separately. Reviewers: Ismael Juma <ismael@juma.me.uk>, José Armando García Sancio <jsancio@users.noreply.github.com>, Jason Gustafson <jason@confluent.io>

…d shutdown (apache#12741) (#59) When the `BrokerServer` starts its shutting down process, it transitions to `SHUTTING_DOWN` and sets `isShuttingDown` to `true`. With this state change, the follower state changes are short-cutted. This means that a broker which was serving as leader would remain acting as a leader until controlled shutdown completes. Instead, we want the leader and ISR state to be updated so that requests will return NOT_LEADER and the client can find the new leader. We missed this case while implementing apache#12187. This patch fixes the issue and updates an existing test to ensure that `isShuttingDown` has not effect. We should consider adding integration tests for this as well. We can do this separately. Reviewers: Ismael Juma <ismael@juma.me.uk>, José Armando García Sancio <jsancio@users.noreply.github.com>, Jason Gustafson <jason@confluent.io> Co-authored-by: David Jacot <djacot@confluent.io>

KAFKA-14296; Partition leaders are not demoted during kraft controlle…

52f1962

…d shutdown

ijuma reviewed Oct 13, 2022

View reviewed changes

hachikuji reviewed Oct 13, 2022

View reviewed changes

hachikuji approved these changes Oct 13, 2022

View reviewed changes

jsancio approved these changes Oct 13, 2022

View reviewed changes

hachikuji merged commit 5cff8f6 into apache:trunk Oct 13, 2022

dajac deleted the KAFKA-14296 branch October 14, 2022 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14296; Partition leaders are not demoted during kraft controlled shutdown#12741

KAFKA-14296; Partition leaders are not demoted during kraft controlled shutdown#12741
hachikuji merged 1 commit intoapache:trunkfrom
dajac:KAFKA-14296

dajac commented Oct 13, 2022

Uh oh!

ijuma Oct 13, 2022

Uh oh!

dajac Oct 13, 2022

Uh oh!

ijuma Oct 13, 2022 •

edited

Loading

Uh oh!

dajac Oct 13, 2022

Uh oh!

hachikuji Oct 13, 2022

Uh oh!

hachikuji left a comment

Uh oh!

jsancio left a comment

Uh oh!

dajac commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

dajac commented Oct 13, 2022

Committer Checklist (excluded from commit message)

Uh oh!

ijuma Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

dajac Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

ijuma Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

hachikuji Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

jsancio left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

ijuma Oct 13, 2022 •

edited

Loading