Kafka
  1. Kafka
  2. KAFKA-1420

Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests

    Details

    • Type: Bug Bug
    • Status: In Progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 0.8.3
    • Component/s: None
    • Labels:

      Description

      This is a follow-up JIRA from KAFKA-1389.

      There are a bunch of places in the unit tests where we misuse AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK to create topics, where TestUtils.createTopic needs to be used instead.

      1. KAFKA-1420.patch
        13 kB
        Jonathan Natkins
      2. KAFKA-1420_2014-08-10_23:03:46.patch
        31 kB
        Jonathan Natkins
      3. KAFKA-1420_2014-08-10_14:12:05.patch
        32 kB
        Jonathan Natkins
      4. KAFKA-1420_2014-08-02_11:04:15.patch
        31 kB
        Jonathan Natkins
      5. KAFKA-1420_2014-07-30_11:24:55.patch
        16 kB
        Jonathan Natkins
      6. KAFKA-1420_2014-07-30_11:18:26.patch
        16 kB
        Jonathan Natkins

        Issue Links

          Activity

          Hide
          Jonathan Natkins added a comment -

          I was starting to work on this JIRA, but I'm hitting a small stumbling block. I've noticed some tests that create brokers using TestUtils.createBrokerConfigs() and mapping over the configs with TestUtils.createServer(), and other tests that create brokers using TestUtils.createBrokersInZk().

          Where this becomes a little confusing is that both implementations of createTopic require a Seq[KafkaServer], but createServer() returns a KafkaServer via a Properties object and createBrokersInZk returns a Seq[Broker], and I don't see a particularly obvious way to go from a Broker to a KakfaServer.

          Am I missing something obvious?

          Show
          Jonathan Natkins added a comment - I was starting to work on this JIRA, but I'm hitting a small stumbling block. I've noticed some tests that create brokers using TestUtils.createBrokerConfigs() and mapping over the configs with TestUtils.createServer(), and other tests that create brokers using TestUtils.createBrokersInZk(). Where this becomes a little confusing is that both implementations of createTopic require a Seq [KafkaServer] , but createServer() returns a KafkaServer via a Properties object and createBrokersInZk returns a Seq [Broker] , and I don't see a particularly obvious way to go from a Broker to a KakfaServer. Am I missing something obvious?
          Hide
          Guozhang Wang added a comment -

          Hi Jonathan,

          TestUtils.createBrokerConfigs() is usually used when the test class is inheriting from KafkaServerTestHarness, which already handles server creation and shutdown at setUp() and tearDown() time; TestUtils.createServer() should then be used otherwise, i.e. when a broker is just needed to be created on the fly.

          TestUtils.createBrokersInZk() is different, though, in that it does not actually create a running server, but just create the registration znode in ZK, the Broker object is just a placeholder for the broker metadata, like broker id, address, etc. They should only be used when we just need to test some ZK-based utilities but do not necessarily need to really create a running server.

          Show
          Guozhang Wang added a comment - Hi Jonathan, TestUtils.createBrokerConfigs() is usually used when the test class is inheriting from KafkaServerTestHarness, which already handles server creation and shutdown at setUp() and tearDown() time; TestUtils.createServer() should then be used otherwise, i.e. when a broker is just needed to be created on the fly. TestUtils.createBrokersInZk() is different, though, in that it does not actually create a running server, but just create the registration znode in ZK, the Broker object is just a placeholder for the broker metadata, like broker id, address, etc. They should only be used when we just need to test some ZK-based utilities but do not necessarily need to really create a running server.
          Hide
          Jonathan Natkins added a comment -

          Thanks for the context, Guozhang. So in the cases where createBrokersInZk is used, should AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK still be used? In particular, I encountered this in AdminTest.testManualReplicaAssignment.

          Show
          Jonathan Natkins added a comment - Thanks for the context, Guozhang. So in the cases where createBrokersInZk is used, should AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK still be used? In particular, I encountered this in AdminTest.testManualReplicaAssignment.
          Hide
          Guozhang Wang added a comment -

          TestUtils.createTopic can be used here also.

          Show
          Guozhang Wang added a comment - TestUtils.createTopic can be used here also.
          Hide
          Jonathan Natkins added a comment - - edited

          Hi Guozhang,

          The particular case I'm confused by is a test like this:

            @Test
            def testManualReplicaAssignment() {
              val brokers = List(0, 1, 2, 3, 4)
              TestUtils.createBrokersInZk(zkClient, brokers)
          
              // duplicate brokers
              intercept[IllegalArgumentException] {
                AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test", Map(0->Seq(0,0)))
              }
          
              // inconsistent replication factor
              intercept[IllegalArgumentException] {
                AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test", Map(0->Seq(0,1), 1->Seq(0)))
              }
          
              // good assignment
              val assignment = Map(0 -> List(0, 1, 2),
                                   1 -> List(1, 2, 3))
              AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test", assignment)
              val found = ZkUtils.getPartitionAssignmentForTopics(zkClient, Seq("test"))
              assertEquals(assignment, found("test"))
            }
          

          This test uses createOrUpdate, but creates brokers via createBrokersInZk, which doesn't return a reference to a KafkaServer.

          The createTopic call, on the other hand, has two implementations:

          def createTopic(zkClient: ZkClient, topic: String, numPartitions: Int = 1, replicationFactor: Int = 1,
                            servers: Seq[KafkaServer])
          
          def createTopic(zkClient: ZkClient, topic: String, partitionReplicaAssignment: collection.Map[Int, Seq[Int]],
                            servers: Seq[KafkaServer])
          

          In both cases, I need a Seq[KafkaServer]. It's unclear to me how to convert the aforementioned test to use createTopic, without materially changing the semantics of the test. Currently it doesn't use a real Kafka server, but to use createTopic would require me to get a hold of some real servers.

          How would a conversion like this be done? Hopefully this clarifies why I'm confused.

          Show
          Jonathan Natkins added a comment - - edited Hi Guozhang, The particular case I'm confused by is a test like this: @Test def testManualReplicaAssignment() { val brokers = List(0, 1, 2, 3, 4) TestUtils.createBrokersInZk(zkClient, brokers) // duplicate brokers intercept[IllegalArgumentException] { AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test" , Map(0->Seq(0,0))) } // inconsistent replication factor intercept[IllegalArgumentException] { AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test" , Map(0->Seq(0,1), 1->Seq(0))) } // good assignment val assignment = Map(0 -> List(0, 1, 2), 1 -> List(1, 2, 3)) AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK(zkClient, "test" , assignment) val found = ZkUtils.getPartitionAssignmentForTopics(zkClient, Seq( "test" )) assertEquals(assignment, found( "test" )) } This test uses createOrUpdate, but creates brokers via createBrokersInZk, which doesn't return a reference to a KafkaServer. The createTopic call, on the other hand, has two implementations: def createTopic(zkClient: ZkClient, topic: String , numPartitions: Int = 1, replicationFactor: Int = 1, servers: Seq[KafkaServer]) def createTopic(zkClient: ZkClient, topic: String , partitionReplicaAssignment: collection.Map[Int, Seq[Int]], servers: Seq[KafkaServer]) In both cases, I need a Seq [KafkaServer] . It's unclear to me how to convert the aforementioned test to use createTopic, without materially changing the semantics of the test. Currently it doesn't use a real Kafka server, but to use createTopic would require me to get a hold of some real servers. How would a conversion like this be done? Hopefully this clarifies why I'm confused.
          Hide
          Guozhang Wang added a comment -

          Hello Jonathan,

          Thanks for the comment. You can note that there are two createTopic functions, one in TestUtils and one in AdminUtils, and TestUtils.createTopic just calls AdminUtils.createTopics and then use the Seq[KafkaServer] to check if the topic metadata has been propagated to these servers.

          For the AdminTest, where no Seq[KafkaServer] is used, we actually do not need the real running servers but just test some ZK-based admin utilities. In this case we can use AdminUtils.createTopic to replace the TestUtils.createOrUpdateTopicPartitionAssignmentPathInZK.

          Show
          Guozhang Wang added a comment - Hello Jonathan, Thanks for the comment. You can note that there are two createTopic functions, one in TestUtils and one in AdminUtils, and TestUtils.createTopic just calls AdminUtils.createTopics and then use the Seq [KafkaServer] to check if the topic metadata has been propagated to these servers. For the AdminTest, where no Seq [KafkaServer] is used, we actually do not need the real running servers but just test some ZK-based admin utilities. In this case we can use AdminUtils.createTopic to replace the TestUtils.createOrUpdateTopicPartitionAssignmentPathInZK.
          Hide
          Jonathan Natkins added a comment -
          Show
          Jonathan Natkins added a comment - Patch available at https://reviews.apache.org/r/24006/
          Hide
          Guozhang Wang added a comment -

          Thanks for the patch, will review it asap.

          Show
          Guozhang Wang added a comment - Thanks for the patch, will review it asap.
          Hide
          Jonathan Natkins added a comment -

          Updated reviewboard https://reviews.apache.org/r/24006/diff/
          against branch origin/trunk

          Show
          Jonathan Natkins added a comment - Updated reviewboard https://reviews.apache.org/r/24006/diff/ against branch origin/trunk
          Hide
          Jonathan Natkins added a comment -

          Updated reviewboard https://reviews.apache.org/r/24006/diff/
          against branch origin/trunk

          Show
          Jonathan Natkins added a comment - Updated reviewboard https://reviews.apache.org/r/24006/diff/ against branch origin/trunk
          Hide
          Jonathan Natkins added a comment -

          Updated reviewboard https://reviews.apache.org/r/24006/diff/
          against branch origin/trunk

          Show
          Jonathan Natkins added a comment - Updated reviewboard https://reviews.apache.org/r/24006/diff/ against branch origin/trunk
          Hide
          Jonathan Natkins added a comment -

          Wanted to try to make sure this doesn't fall too far off the radar, since the review is mostly done.

          Show
          Jonathan Natkins added a comment - Wanted to try to make sure this doesn't fall too far off the radar, since the review is mostly done.
          Hide
          Guozhang Wang added a comment -

          Sorry Jonathan, was completely swamped the past week. Will get back to you asap.

          Show
          Guozhang Wang added a comment - Sorry Jonathan, was completely swamped the past week. Will get back to you asap.
          Hide
          Jonathan Natkins added a comment -

          Updated reviewboard https://reviews.apache.org/r/24006/diff/
          against branch origin/trunk

          Show
          Jonathan Natkins added a comment - Updated reviewboard https://reviews.apache.org/r/24006/diff/ against branch origin/trunk
          Hide
          Jonathan Natkins added a comment -

          Updated reviewboard https://reviews.apache.org/r/24006/diff/
          against branch origin/trunk

          Show
          Jonathan Natkins added a comment - Updated reviewboard https://reviews.apache.org/r/24006/diff/ against branch origin/trunk
          Hide
          Neha Narkhede added a comment -

          Assigning to myself for review

          Show
          Neha Narkhede added a comment - Assigning to myself for review
          Hide
          Neha Narkhede added a comment -

          Doesn't look like I'm going to be able to get to this in the next couple days. Reassigning to Jun Rao to see if he has some time to review this.

          Show
          Neha Narkhede added a comment - Doesn't look like I'm going to be able to get to this in the next couple days. Reassigning to Jun Rao to see if he has some time to review this.
          Hide
          Jonathan Natkins added a comment -

          Wanted to bump this ticket so the patch doesn't get irreparably stale.

          Show
          Jonathan Natkins added a comment - Wanted to bump this ticket so the patch doesn't get irreparably stale.
          Hide
          Guozhang Wang added a comment -

          Can some committer take another look and commit this one? It looks good to me now.

          Show
          Guozhang Wang added a comment - Can some committer take another look and commit this one? It looks good to me now.
          Hide
          Neha Narkhede added a comment -

          Jun Rao, please feel free to reassign for review.

          Show
          Neha Narkhede added a comment - Jun Rao , please feel free to reassign for review.
          Hide
          Gwen Shapira added a comment -

          This probably needs rebase.

          Jonathan Natkins would you have time to rebase?
          Guozhang Wang will you be ok committing this once Natty rebases?

          Show
          Gwen Shapira added a comment - This probably needs rebase. Jonathan Natkins would you have time to rebase? Guozhang Wang will you be ok committing this once Natty rebases?

            People

            • Assignee:
              Jonathan Natkins
              Reporter:
              Guozhang Wang
              Reviewer:
              Jun Rao
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development