Kafka
  1. Kafka
  2. KAFKA-779

Standardize Zk data structures for Re-assign partitions and Preferred replication election

    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-779-v3.patch
      19 kB
      Swapnil Ghike
    2. kafka-779-v2.patch
      19 kB
      Swapnil Ghike
    3. kafka-779-v1.patch
      18 kB
      Swapnil Ghike

      Activity

      Swapnil Ghike created issue -
      Neha Narkhede made changes -
      Field Original Value New Value
      Labels p2 kafka-0.8 p2
      Hide
      Swapnil Ghike added a comment - - edited

      Changes:

      1. Zk data format standardization for admin/reassign_partitions and admin/preffered_replica_election. (These changes still hit the bug mentioned in KAFKA-780 which occurred without the changes in this patch.)

      2. I changed Json manipulation related functions in Utils, because mergeJsonObjects had a bug which could screw up a JSON string that included a nested object or an array, while attempting to sort the fields in the string. I replaced it with a new function mergeJsonFields and created a new function mapToJsonFields. As it turned out, these two functions also helped reduce some lines of code in other JSON related functions in Utils.

      3. Ran optimize imports on the entire source code. Three files unrelated to this patch have been touched.

      Testing done -
      i. Unit tests pass
      ii. Verified the format of all Zk data structures as mentioned in the wiki to ensure 2 above didn't break anything. Verified that the fields are sorted.
      iii. Tested reassign-partitions tool and preferred-replica-election admin tools, but they hit the exception mentioned in KAFKA-780.

      Show
      Swapnil Ghike added a comment - - edited Changes: 1. Zk data format standardization for admin/reassign_partitions and admin/preffered_replica_election. (These changes still hit the bug mentioned in KAFKA-780 which occurred without the changes in this patch.) 2. I changed Json manipulation related functions in Utils, because mergeJsonObjects had a bug which could screw up a JSON string that included a nested object or an array, while attempting to sort the fields in the string. I replaced it with a new function mergeJsonFields and created a new function mapToJsonFields. As it turned out, these two functions also helped reduce some lines of code in other JSON related functions in Utils. 3. Ran optimize imports on the entire source code. Three files unrelated to this patch have been touched. Testing done - i. Unit tests pass ii. Verified the format of all Zk data structures as mentioned in the wiki to ensure 2 above didn't break anything. Verified that the fields are sorted. iii. Tested reassign-partitions tool and preferred-replica-election admin tools, but they hit the exception mentioned in KAFKA-780 .
      Swapnil Ghike made changes -
      Attachment kafka-779-v1.patch [ 12571841 ]
      Hide
      Swapnil Ghike added a comment -

      Modified the description of the --path-to-json-file option required by the tools.

      Show
      Swapnil Ghike added a comment - Modified the description of the --path-to-json-file option required by the tools.
      Swapnil Ghike made changes -
      Attachment kafka-779-v2.patch [ 12571963 ]
      Hide
      Jun Rao added a comment -

      Thanks for patch v2. Looks good overall. A couple of comments:

      1. PreferredReplicaLeaderElectionCommand: We shouldn't let the user put in the version # in path-to-json-file. The tool knows the version and should fill it in. Ditto for ReassignPartitionsCommand.

      2. I had the following unit test failure.
      [error] Test Failed: testPartitionReassignmentWithLeaderInNewReplicas(kafka.admin.AdminTest)
      junit.framework.AssertionFailedError: Partition should have been reassigned to 0, 2, 3 expected:<List(0, 2, 3)> but was:<List(0, 1, 2)>
      at junit.framework.Assert.fail(Assert.java:47)
      at junit.framework.Assert.failNotEquals(Assert.java:277)
      at junit.framework.Assert.assertEquals(Assert.java:64)
      at kafka.admin.AdminTest.testPartitionReassignmentWithLeaderInNewReplicas(AdminTest.scala:232)

      Show
      Jun Rao added a comment - Thanks for patch v2. Looks good overall. A couple of comments: 1. PreferredReplicaLeaderElectionCommand: We shouldn't let the user put in the version # in path-to-json-file. The tool knows the version and should fill it in. Ditto for ReassignPartitionsCommand. 2. I had the following unit test failure. [error] Test Failed: testPartitionReassignmentWithLeaderInNewReplicas(kafka.admin.AdminTest) junit.framework.AssertionFailedError: Partition should have been reassigned to 0, 2, 3 expected:<List(0, 2, 3)> but was:<List(0, 1, 2)> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:277) at junit.framework.Assert.assertEquals(Assert.java:64) at kafka.admin.AdminTest.testPartitionReassignmentWithLeaderInNewReplicas(AdminTest.scala:232)
      Hide
      Swapnil Ghike added a comment -

      Patch v3:

      1. Ok, not asking the user to provide version in both the tools.

      2. Seems like this was a transient failure. I ran it multiple times on my machine, it passed. We can take a look at it if it fails more often than not.

      Show
      Swapnil Ghike added a comment - Patch v3: 1. Ok, not asking the user to provide version in both the tools. 2. Seems like this was a transient failure. I ran it multiple times on my machine, it passed. We can take a look at it if it fails more often than not.
      Swapnil Ghike made changes -
      Attachment kafka-779-v3.patch [ 12572385 ]
      Hide
      Jun Rao added a comment -

      Thanks for patch v3. Committed to 0.8.

      Show
      Jun Rao added a comment - Thanks for patch v3. Committed to 0.8.
      Jun Rao made changes -
      Status Open [ 1 ] Resolved [ 5 ]
      Resolution Fixed [ 1 ]
      Hide
      Swapnil Ghike added a comment -

      Jay had a great suggestion about combining Json encoding utils into one single util. He has already checked in the code to trunk at KAFKA-554. So, perhaps we should make the related refactoring in trunk. I would be happy to do it.

      Show
      Swapnil Ghike added a comment - Jay had a great suggestion about combining Json encoding utils into one single util. He has already checked in the code to trunk at KAFKA-554 . So, perhaps we should make the related refactoring in trunk. I would be happy to do it.
      Neha Narkhede made changes -
      Status Resolved [ 5 ] Closed [ 6 ]

        People

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

          Dates

          • Created:
            Updated:
            Resolved:

            Development