Kafka
  1. Kafka
  2. KAFKA-830

partition replica assignment map in the controller should be a Set

    Details

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

      Description

      partitionReplicaAssignment currently stores the list of assigned replicas as a sequence. When a broker comes online, the replica state machine adds the broker to the list of assigned replicas. It should do that only if the replica is already not in the list of assigned replicas. This causes the replication factor to be incorrectly calculated

      1. kafka-830-v1.patch
        26 kB
        Neha Narkhede
      2. kafka-830-v2.patch
        1 kB
        Neha Narkhede

        Activity

        Hide
        Neha Narkhede added a comment -
        • Changed any replica list to a set. This makes sense since replica ids can never legitimately repeat.
        Show
        Neha Narkhede added a comment - Changed any replica list to a set. This makes sense since replica ids can never legitimately repeat.
        Hide
        Jun Rao added a comment -

        Thanks for the patch. One problem with changing assigned replicas from a list to a set is that it breaks the preferred replica logic. We assume that the first replica is the preferred one. So the ordering of the replicas is important. Perhaps we should keep replicas as a list and make sure that we don't add duplicates when changing it.

        Show
        Jun Rao added a comment - Thanks for the patch. One problem with changing assigned replicas from a list to a set is that it breaks the preferred replica logic. We assume that the first replica is the preferred one. So the ordering of the replicas is important. Perhaps we should keep replicas as a list and make sure that we don't add duplicates when changing it.
        Hide
        Neha Narkhede added a comment -

        Good catch! However, it doesn't make sense to keep it sequence since by definition those ids are unique. As for preferred replica logic, currently it is just the smallest replica id . So there are 2 options -

        1. Store replica ids in a sorted set
        2. For preferred replica, pick the smallest id from a traditional hash set

        But in patch v2, I stuck to your suggestion since it is a much smaller change for 0.8. Will fix this in 0.8.1

        Show
        Neha Narkhede added a comment - Good catch! However, it doesn't make sense to keep it sequence since by definition those ids are unique. As for preferred replica logic, currently it is just the smallest replica id . So there are 2 options - 1. Store replica ids in a sorted set 2. For preferred replica, pick the smallest id from a traditional hash set But in patch v2, I stuck to your suggestion since it is a much smaller change for 0.8. Will fix this in 0.8.1
        Hide
        Neha Narkhede added a comment -

        Actually, thinking about it more, even picking the smallest replica id does not make sense. This is because if we pick the first replica to be the largest broker id, the rest of the replica ids are smaller than the preferred replica in this case. We will have to keep this a sequence and be careful while adding a replica id to the assigned replica list

        Show
        Neha Narkhede added a comment - Actually, thinking about it more, even picking the smallest replica id does not make sense. This is because if we pick the first replica to be the largest broker id, the rest of the replica ids are smaller than the preferred replica in this case. We will have to keep this a sequence and be careful while adding a replica id to the assigned replica list
        Hide
        Swapnil Ghike added a comment -

        collection.mutable.LinkedHashSet will iterate the elements in the same sequence they were inserted. But using it will require changes at more places than the one line change in patch v2.

        Show
        Swapnil Ghike added a comment - collection.mutable.LinkedHashSet will iterate the elements in the same sequence they were inserted. But using it will require changes at more places than the one line change in patch v2.
        Hide
        Jun Rao added a comment -

        +1 on patch v2.

        Show
        Jun Rao added a comment - +1 on patch v2.
        Hide
        Neha Narkhede added a comment -

        Thanks for the review

        Show
        Neha Narkhede added a comment - Thanks for the review
        Hide
        Neha Narkhede added a comment -

        Swapnil, thanks for pointing that out, we can remember to do this in 0.8.1

        Show
        Neha Narkhede added a comment - Swapnil, thanks for pointing that out, we can remember to do this in 0.8.1

          People

          • Assignee:
            Neha Narkhede
            Reporter:
            Neha Narkhede
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development