You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Jonathan Natkins <na...@cloudera.com> on 2014/07/28 22:45:29 UTC

Review Request 24006: Patch for KAFKA-1420

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in all unit tests


Diffs
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.


> On July 30, 2014, 12:22 a.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 314
> > <https://reviews.apache.org/r/24006/diff/1/?file=643839#file643839line314>
> >
> >     Is there a specific reason we want to use 10 seconds instead of default 5 seconds?

Sorry, I'd added this in the midst of debugging, and forgotten to remove it. I've actually changed this call, because I realized that it didn't necessarily assure me that broker 0 had caught up to the ISR yet. The test has been changed to be more reliable.


> On July 30, 2014, 12:22 a.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 317
> > <https://reviews.apache.org/r/24006/diff/1/?file=643839#file643839line317>
> >
> >     Is this println intended?

Removed


> On July 30, 2014, 12:22 a.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/utils/TestUtils.scala, line 186
> > <https://reviews.apache.org/r/24006/diff/1/?file=643842#file643842line186>
> >
> >     Could we just set the default value of configs parameter to null, instead of creating a separate function?

The reason I'd done this is that the Scala compiler complained because there's another implementation of createTopic that defines default parameter values. However, I was able to change the calls to this API to the one that uses numPartitions and replicationFactor, and stuck the Properties parameter over there.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review49049
-----------------------------------------------------------


On July 30, 2014, 6:18 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated July 30, 2014, 6:18 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Guozhang Wang <gu...@linkedin.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review49049
-----------------------------------------------------------



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85879>

    Could you group kafka imports together before java/scala/other-libs imports?



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85878>

    Could we use 
    
    val numPartitions = 12
    val replicationFactor = 3
    
    and then create expectedReplicaAssignment and leaderForPartitionMap based on these two variables, and re-use them here?



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85880>

    expectedReplicaAssignment seems not used any more.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85888>

    Could you add a comment here for bouncing server 1?



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85890>

    Is there a specific reason we want to use 10 seconds instead of default 5 seconds?



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85889>

    Is this println intended?



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/24006/#comment85891>

    Do we still need expectedReplicaAssignment?



core/src/test/scala/unit/kafka/utils/TestUtils.scala
<https://reviews.apache.org/r/24006/#comment85892>

    Could we just set the default value of configs parameter to null, instead of creating a separate function?


- Guozhang Wang


On July 28, 2014, 8:52 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated July 28, 2014, 8:52 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in all unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.


> On Aug. 1, 2014, 9:27 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 134
> > <https://reviews.apache.org/r/24006/diff/3/?file=646111#file646111line134>
> >
> >     What I was actually thinking is that probably we can define these two variables at the top of testTopicCreationInZK and crate the expectedReplicaAssignment and leaderForPartitionMap programmatically instead of hand-written them.

This would admittedly be easier in later versions of Scala, which have some combinatorial functionality, but I've written a little code to do this. Take a look at it...I'm not totally sure it's what you had in mind, so let me know if you just meant for me to use AdminTools.assignReplicasToBrokers or something like that.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review49382
-----------------------------------------------------------


On Aug. 2, 2014, 6:04 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2014, 6:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Guozhang Wang <gu...@linkedin.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review49382
-----------------------------------------------------------



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment86410>

    RunningAsBroker is not used any more.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment86411>

    What I was actually thinking is that probably we can define these two variables at the top of testTopicCreationInZK and crate the expectedReplicaAssignment and leaderForPartitionMap programmatically instead of hand-written them.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment86413>

    After reading this test more clearly, I think it was originally trying to "testResumePartitionReassignmentThatWasIncomplet", i.e. when the brokers are started up they need to finish whatever reassignment tasks is left on ZK. In this case, we cannot create the brokers before hand.
    
    What we can actually do, is to 1) create the brokers and create the topics, 2) shutdown brokers, 3) write the reassignment in ZK, 4) restart-brokers.



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/24006/#comment86420>

    Could we rename these to "replicaIdsForPartition" and "serversHostingPartition"?



core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala
<https://reviews.apache.org/r/24006/#comment86421>

    We do not need to declare these variables if they are not re-usable other than the createTopic() call. Same blew.


- Guozhang Wang


On July 30, 2014, 6:24 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated July 30, 2014, 6:24 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@streamsets.com>.

Ah, gotcha. Given that, I think I made the right adjustment. Thanks for the
clarification!


On Sun, Aug 10, 2014 at 10:38 PM, Guozhang Wang <gu...@linkedin.com> wrote:

>
>
> > On Aug. 10, 2014, 9:12 p.m., Jonathan Natkins wrote:
> > > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 114
> > > <
> https://reviews.apache.org/r/24006/diff/3-4/?file=646111#file646111line114
> >
> > >
> > >     I wasn't totally sure I understood this comment, so I made a
> change that I think reflects what you were looking for. Let me know if I
> missed the mark.
>
> What I meant is that we need to guarantee the preferred replica would be
> the first replica in the list. For our case, it just that
>
> range(0, 10).map(i => (i -> i % brokers.size)).toMap
>
> gets the same result that it gets the first replica of the lists returned
> by
>
> combinations(brokers, replicationFactor)
>
> but it may not always be the case.
>
>
> - Guozhang
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/#review50126
> -----------------------------------------------------------
>
>
> On Aug. 10, 2014, 9:11 p.m., Jonathan Natkins wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/24006/
> > -----------------------------------------------------------
> >
> > (Updated Aug. 10, 2014, 9:11 p.m.)
> >
> >
> > Review request for kafka.
> >
> >
> > Bugs: KAFKA-1420
> >     https://issues.apache.org/jira/browse/KAFKA-1420
> >
> >
> > Repository: kafka
> >
> >
> > Description
> > -------
> >
> > KAFKA-1420 Replace
> AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with
> TestUtils.createTopic in unit tests
> >
> >
> > Diffs
> > -----
> >
> >   core/src/test/scala/unit/kafka/admin/AdminTest.scala
> e28979827110dfbbb92fe5b152e7f1cc973de400
> >   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
> 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc
> >
> core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala
> f44568cb25edf25db857415119018fd4c9922f61
> >   core/src/test/scala/unit/kafka/utils/TestUtils.scala
> c4e13c5240c8303853d08cc3b40088f8c7dae460
> >
> > Diff: https://reviews.apache.org/r/24006/diff/
> >
> >
> > Testing
> > -------
> >
> > Automated
> >
> >
> > Thanks,
> >
> > Jonathan Natkins
> >
> >
>
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Guozhang Wang <gu...@linkedin.com>.


> On Aug. 10, 2014, 9:12 p.m., Jonathan Natkins wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 114
> > <https://reviews.apache.org/r/24006/diff/3-4/?file=646111#file646111line114>
> >
> >     I wasn't totally sure I understood this comment, so I made a change that I think reflects what you were looking for. Let me know if I missed the mark.

What I meant is that we need to guarantee the preferred replica would be the first replica in the list. For our case, it just that

range(0, 10).map(i => (i -> i % brokers.size)).toMap

gets the same result that it gets the first replica of the lists returned by

combinations(brokers, replicationFactor)

but it may not always be the case.


- Guozhang


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review50126
-----------------------------------------------------------


On Aug. 10, 2014, 9:11 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 9:11 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review50126
-----------------------------------------------------------



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment87702>

    I wasn't totally sure I understood this comment, so I made a change that I think reflects what you were looking for. Let me know if I missed the mark.


- Jonathan Natkins


On Aug. 10, 2014, 9:11 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 9:11 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review50137
-----------------------------------------------------------


Thanks for the patch. One comment below.


core/src/test/scala/unit/kafka/utils/TestUtils.scala
<https://reviews.apache.org/r/24006/#comment87735>

    Now that KAFKA-1419 is committed, could we remove this?


- Jun Rao


On Aug. 10, 2014, 9:11 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 9:11 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Guozhang Wang <gu...@linkedin.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review50264
-----------------------------------------------------------


Looks good to me. Can other committers double-check it?

- Guozhang Wang


On Aug. 11, 2014, 6:03 a.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2014, 6:03 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review80620
-----------------------------------------------------------


Thanks for the patch. Sorry for the late review. A few comments below.


core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment130720>

    This is a case where we probably want to explicitly specify the replica assignment. We don't want to depend on the current assignment strategy since it may change in the future.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment130721>

    Ditto as the above.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment130722>

    Ditto as the above.



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment130728>

    This may not be a reliable way to wait for replica 0 to be in isr. Perhaps, we can explicitly check the isr set in Partition through ReplicaManager in the leader broker.



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala
<https://reviews.apache.org/r/24006/#comment130731>

    Should we use replicaIdsForPartition instead of Seq(0,1,2,3)?


- Jun Rao


On Aug. 11, 2014, 6:03 a.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2014, 6:03 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated Aug. 11, 2014, 6:03 a.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests


Diffs (updated)
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------

Automated


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated Aug. 10, 2014, 9:11 p.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests


Diffs (updated)
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------

Automated


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Guozhang Wang <gu...@linkedin.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review50060
-----------------------------------------------------------

Ship it!


Looks good to me, just a couple minor comments below.


core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment87581>

    Could you move this function to TestUtils?



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment87584>

    we can probably just use sth. replicaSets.map(_.get(0)), otherwise we have to make sure that the leaders are correctly set.


- Guozhang Wang


On Aug. 2, 2014, 6:04 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2014, 6:04 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated Aug. 2, 2014, 6:04 p.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests


Diffs (updated)
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------

Automated


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/#review49139
-----------------------------------------------------------



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85979>

    Will fix whitespace



core/src/test/scala/unit/kafka/admin/AdminTest.scala
<https://reviews.apache.org/r/24006/#comment85980>

    Will fix whitespace here


- Jonathan Natkins


On July 30, 2014, 6:24 p.m., Jonathan Natkins wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24006/
> -----------------------------------------------------------
> 
> (Updated July 30, 2014, 6:24 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1420
>     https://issues.apache.org/jira/browse/KAFKA-1420
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
>   core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 
> 
> Diff: https://reviews.apache.org/r/24006/diff/
> 
> 
> Testing
> -------
> 
> Automated
> 
> 
> Thanks,
> 
> Jonathan Natkins
> 
>

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated July 30, 2014, 6:24 p.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests


Diffs (updated)
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------

Automated


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated July 30, 2014, 6:18 p.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description (updated)
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in unit tests


Diffs (updated)
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing
-------

Automated


Thanks,

Jonathan Natkins

Re: Review Request 24006: Patch for KAFKA-1420

Posted by Jonathan Natkins <na...@cloudera.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24006/
-----------------------------------------------------------

(Updated July 28, 2014, 8:52 p.m.)


Review request for kafka.


Bugs: KAFKA-1420
    https://issues.apache.org/jira/browse/KAFKA-1420


Repository: kafka


Description
-------

KAFKA-1420 Replace AdminUtils.createOrUpdateTopicPartitionAssignmentPathInZK with TestUtils.createTopic in all unit tests


Diffs
-----

  core/src/test/scala/unit/kafka/admin/AdminTest.scala e28979827110dfbbb92fe5b152e7f1cc973de400 
  core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala 29cc01bcef9cacd8dec1f5d662644fc6fe4994bc 
  core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala f44568cb25edf25db857415119018fd4c9922f61 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala c4e13c5240c8303853d08cc3b40088f8c7dae460 

Diff: https://reviews.apache.org/r/24006/diff/


Testing (updated)
-------

Automated


Thanks,

Jonathan Natkins