Kafka
  1. Kafka
  2. KAFKA-840

Controller tries to perform preferred replica election on failover before state machines have started up

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: controller
    • Labels:

      Description

      If the admin/preferred_replica_election path is non-empty when a new controller starts, the controller attempts to perform preferred replica election before the partition and replica state machine have been initialized. In this case, the controller will try to make invalid state transitions.

      1. kafka-840-v2.patch
        8 kB
        Swapnil Ghike
      2. kafka-840-after-commit-v2.patch
        6 kB
        Swapnil Ghike
      3. kafka-840-after-commit-v1.patch
        6 kB
        Swapnil Ghike
      4. kafka-840.patch
        1 kB
        Swapnil Ghike

        Activity

        Hide
        Swapnil Ghike added a comment -

        I think the right solution is to move initializeAndMaybeTriggerPartitionReassignment() and initializeAndMaybeTriggerPreferredReplicaElection() after the controller mbean is registered and the controller logs that it is ready to serve. Attached a patch.

        Show
        Swapnil Ghike added a comment - I think the right solution is to move initializeAndMaybeTriggerPartitionReassignment() and initializeAndMaybeTriggerPreferredReplicaElection() after the controller mbean is registered and the controller logs that it is ready to serve. Attached a patch.
        Hide
        Neha Narkhede added a comment -

        Thanks for the patch. In addition to fixing the issue at hand, I think it will be good to throw an IllegalOperationException if handleStateChanges() is invoked before startup() or after shutdown() in both PartitionStateMachine and ReplicaStateMachine

        Show
        Neha Narkhede added a comment - Thanks for the patch. In addition to fixing the issue at hand, I think it will be good to throw an IllegalOperationException if handleStateChanges() is invoked before startup() or after shutdown() in both PartitionStateMachine and ReplicaStateMachine
        Hide
        Swapnil Ghike added a comment -

        1. Renamed inShuttingdown in partition/replica state machines to isRunning, because the former does not let us distinguish between a state machine that has not started, and a state machine that has started and is not shutting down. Made changes at other places accordingly.

        2. With this patch, if the state machine has not started, then handleStateChange() throws a StateChangeFailedException with a message "because the state machine has not started".

        Show
        Swapnil Ghike added a comment - 1. Renamed inShuttingdown in partition/replica state machines to isRunning, because the former does not let us distinguish between a state machine that has not started, and a state machine that has started and is not shutting down. Made changes at other places accordingly. 2. With this patch, if the state machine has not started, then handleStateChange() throws a StateChangeFailedException with a message "because the state machine has not started".
        Hide
        Neha Narkhede added a comment -

        Cool fix for handling the "not started", "started but shutdown" conditions. +1
        It's ok to not respond to new topic/new broker zookeeper watches, since on controller failover, the new controller will read the new topics/brokers and put them in New state.

        Show
        Neha Narkhede added a comment - Cool fix for handling the "not started", "started but shutdown" conditions. +1 It's ok to not respond to new topic/new broker zookeeper watches, since on controller failover, the new controller will read the new topics/brokers and put them in New state.
        Hide
        Neha Narkhede added a comment -

        Patch v2 is checked in, thanks Swapnil!

        Show
        Neha Narkhede added a comment - Patch v2 is checked in, thanks Swapnil!
        Hide
        Swapnil Ghike added a comment -

        I made a mistake. Patch v2 ignores a broker/topic change listener callback if the state machines have not started up. However, if such a callback occurs after the controller context has been initialized in controller failover, the topic/broker change will not be noticed by the state machines.

        The right fix is to have to two separate flags in the state machines - hasStarted and hasShutdown.

        Show
        Swapnil Ghike added a comment - I made a mistake. Patch v2 ignores a broker/topic change listener callback if the state machines have not started up. However, if such a callback occurs after the controller context has been initialized in controller failover, the topic/broker change will not be noticed by the state machines. The right fix is to have to two separate flags in the state machines - hasStarted and hasShutdown.
        Hide
        Neha Narkhede added a comment -

        Thanks for the patch! One issue is that the !hasShutdown check should be within the lock, otherwise the state machine can end up trying to respond to a zookeeper listener even after it has shutdown. Today, this cannot happen since we close the zkclient before the controller shuts down, but this is a defensive check which will protect us if the shutdown order changes in the future.

        Show
        Neha Narkhede added a comment - Thanks for the patch! One issue is that the !hasShutdown check should be within the lock, otherwise the state machine can end up trying to respond to a zookeeper listener even after it has shutdown. Today, this cannot happen since we close the zkclient before the controller shuts down, but this is a defensive check which will protect us if the shutdown order changes in the future.
        Hide
        Swapnil Ghike added a comment -

        Makes sense. Should we also de-register the topic/broker change listeners when the partition/replica state machines are shutting down?

        Show
        Swapnil Ghike added a comment - Makes sense. Should we also de-register the topic/broker change listeners when the partition/replica state machines are shutting down?
        Hide
        Neha Narkhede added a comment -

        That would make sense, but with the current shutdown logic of the server, zkclient is closed before controller is shutdown. So doing that will raise an error.

        Show
        Neha Narkhede added a comment - That would make sense, but with the current shutdown logic of the server, zkclient is closed before controller is shutdown. So doing that will raise an error.
        Hide
        Swapnil Ghike added a comment -

        Cool, attached a new patch 'after-commit-v2'.

        Show
        Swapnil Ghike added a comment - Cool, attached a new patch 'after-commit-v2'.
        Hide
        Neha Narkhede added a comment -

        +1 on the latest patch, thanks Swapnil!

        Show
        Neha Narkhede added a comment - +1 on the latest patch, thanks Swapnil!
        Hide
        Swapnil Ghike added a comment -

        Did we check in the 'after-commit-v2' patch? Thanks.

        Show
        Swapnil Ghike added a comment - Did we check in the 'after-commit-v2' patch? Thanks.
        Hide
        Neha Narkhede added a comment -

        Committed 'after-commit-v2' patch to 0.8

        Show
        Neha Narkhede added a comment - Committed 'after-commit-v2' patch to 0.8

          People

          • Assignee:
            Swapnil Ghike
            Reporter:
            Swapnil Ghike
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development