Kafka
  1. Kafka
  2. KAFKA-828

Preferred Replica Election does not delete the admin path on controller failover

    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: None
    • Labels:
    1. kafka-828-v4.patch
      12 kB
      Swapnil Ghike
    2. kafka-828-v3.patch
      9 kB
      Swapnil Ghike
    3. kafka-828-v2.patch
      9 kB
      Swapnil Ghike
    4. kafka-828-v1.patch
      8 kB
      Swapnil Ghike

      Activity

      Swapnil Ghike created issue -
      Swapnil Ghike made changes -
      Field Original Value New Value
      Summary Preferred Replica Election does not delete the admin on controller failover Preferred Replica Election does not delete the admin path on controller failover
      Neha Narkhede made changes -
      Labels bugs bugs kafka-0.8 p1
      Hide
      Swapnil Ghike added a comment -

      1. Moving removePartitionsFromPreferredReplicaElection() to onPreferredReplicaElection() so that when a controller fails over and tries to complete the preferred replica election, the admin path is deleted.

      2. Moved the parsePreferredReplicaElectionData() to ZkUtils from the admin.PreferredReplicaLeaderElectionCommand. There was also another function in ZkUtils for the same purpose and it was outdated, removed it.

      Show
      Swapnil Ghike added a comment - 1. Moving removePartitionsFromPreferredReplicaElection() to onPreferredReplicaElection() so that when a controller fails over and tries to complete the preferred replica election, the admin path is deleted. 2. Moved the parsePreferredReplicaElectionData() to ZkUtils from the admin.PreferredReplicaLeaderElectionCommand. There was also another function in ZkUtils for the same purpose and it was outdated, removed it.
      Swapnil Ghike made changes -
      Attachment kafka-828-v1.patch [ 12575600 ]
      Hide
      Swapnil Ghike added a comment -

      Converted KAFKA-814 to subtask of this as Sriram asked to. Added a one line change to PreferredReplicaLeaderSelector to log instead of throwing an exception if the preferred leader is already the current leader.

      Show
      Swapnil Ghike added a comment - Converted KAFKA-814 to subtask of this as Sriram asked to. Added a one line change to PreferredReplicaLeaderSelector to log instead of throwing an exception if the preferred leader is already the current leader.
      Swapnil Ghike made changes -
      Attachment kafka-828-v2.patch [ 12575601 ]
      Hide
      Swapnil Ghike added a comment -

      Actually since preferred replica election is not a heavy weight operation like partition reassignment (where we need to wait until the new followers catch up etc), we can move reading of the controllerContext.partitionsUndergoingPreferredReplicaElection to inside the controllerLock. Attached patch v3.

      Show
      Swapnil Ghike added a comment - Actually since preferred replica election is not a heavy weight operation like partition reassignment (where we need to wait until the new followers catch up etc), we can move reading of the controllerContext.partitionsUndergoingPreferredReplicaElection to inside the controllerLock. Attached patch v3.
      Swapnil Ghike made changes -
      Attachment kafka-828-v3.patch [ 12575639 ]
      Hide
      Jun Rao added a comment -

      Thanks for patch v3. A couple of comments:

      1. There is a compilation error
      [error] /Users/jrao/intellij_workspace/kafka_git/core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala:129: type mismatch;
      [error] found : Unit
      [error] required: (kafka.api.LeaderAndIsr, Seq[Int])
      [error] info("Preferred replica %d is already the current leader for partition %s".format(preferredReplica, topicAndPartition))
      [error] ^
      [error] one error found

      2. PreferredReplicaPartitionLeaderSelector: Is there any value in logging the following?
      info("Preferred replica %d is already the current leader for partition %s".format(preferredReplica, topicAndPartition))
      The same info will be logged in KafkaController.removePartitionsFromPreferredReplicaElection().

      3. ZkUtils.parsePreferredReplicaElectionData: This is very specific to preferred replica election. So, it's better to move it to PreferredReplicaLeaderElectionCommand.

      Show
      Jun Rao added a comment - Thanks for patch v3. A couple of comments: 1. There is a compilation error [error] /Users/jrao/intellij_workspace/kafka_git/core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala:129: type mismatch; [error] found : Unit [error] required: (kafka.api.LeaderAndIsr, Seq [Int] ) [error] info("Preferred replica %d is already the current leader for partition %s".format(preferredReplica, topicAndPartition)) [error] ^ [error] one error found 2. PreferredReplicaPartitionLeaderSelector: Is there any value in logging the following? info("Preferred replica %d is already the current leader for partition %s".format(preferredReplica, topicAndPartition)) The same info will be logged in KafkaController.removePartitionsFromPreferredReplicaElection(). 3. ZkUtils.parsePreferredReplicaElectionData: This is very specific to preferred replica election. So, it's better to move it to PreferredReplicaLeaderElectionCommand.
      Hide
      Neha Narkhede added a comment -

      Agree with Jun on 1, 2 and 3.

      The issue with your change to the leader selector is that it doesn't achieve the objective of KAFKA-814 completely. If a preferred replica is already the leader, we do not want to update zookeeper and send the become leader request to the replica again. It should just continue to work on the next partition. It doesn't achieve that. One way of handling that is throwing a special exception LeaderAlreadyElectedException (or a better name) and then catching and swallowing it inside electLeaderForPartition()

      Show
      Neha Narkhede added a comment - Agree with Jun on 1, 2 and 3. The issue with your change to the leader selector is that it doesn't achieve the objective of KAFKA-814 completely. If a preferred replica is already the leader, we do not want to update zookeeper and send the become leader request to the replica again. It should just continue to work on the next partition. It doesn't achieve that. One way of handling that is throwing a special exception LeaderAlreadyElectedException (or a better name) and then catching and swallowing it inside electLeaderForPartition()
      Hide
      Swapnil Ghike added a comment -

      Agree with all comments, uploading patch v4.

      Defined a new LeaderElectionNotNeededException that is thrown in selectLeader() when the current leader is the same as the preferred replica. electLeaderForPartition() swallows it.

      Thanks for the reviews and sorry, I was quite lousy while including the patch for 814.

      Show
      Swapnil Ghike added a comment - Agree with all comments, uploading patch v4. Defined a new LeaderElectionNotNeededException that is thrown in selectLeader() when the current leader is the same as the preferred replica. electLeaderForPartition() swallows it. Thanks for the reviews and sorry, I was quite lousy while including the patch for 814.
      Swapnil Ghike made changes -
      Attachment kafka-828-v4.patch [ 12575835 ]
      Hide
      Neha Narkhede added a comment -

      Thanks for the patch ! +1

      Show
      Neha Narkhede added a comment - Thanks for the patch ! +1
      Hide
      Neha Narkhede added a comment -

      Committed patch v4

      Show
      Neha Narkhede added a comment - Committed patch v4
      Neha Narkhede made changes -
      Status Open [ 1 ] Resolved [ 5 ]
      Resolution Fixed [ 1 ]
      Neha Narkhede made changes -
      Status Resolved [ 5 ] Closed [ 6 ]

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development