Re: Review Request 24006: Patch for KAFKA-1420

[Jun Rao]

---
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


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


Ditto as the above.



core/src/test/scala/unit/kafka/admin/AdminTest.scala


Ditto as the above.



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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


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

[Guozhang Wang]

---
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

[Jonathan Natkins]

---
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

[Jonathan Natkins]

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  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

[Guozhang Wang]

> On Aug. 10, 2014, 9:12 p.m., Jonathan Natkins wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 114
> > 
> >
> > 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

[Jun Rao]

---
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


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

[Jonathan Natkins]

---
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

[Jonathan Natkins]

---
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


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

[Guozhang Wang]

---
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


Could you move this function to TestUtils?



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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

[Jonathan Natkins]

> On Aug. 1, 2014, 9:27 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 134
> > 
> >
> > 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

[Jonathan Natkins]

---
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

[Guozhang Wang]

---
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


RunningAsBroker is not used any more.



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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


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


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



core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala


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

[Jonathan Natkins]

---
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


Will fix whitespace



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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

[Jonathan Natkins]

> On July 30, 2014, 12:22 a.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/admin/AdminTest.scala, line 314
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Jonathan Natkins]

---
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

[Jonathan Natkins]

---
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

[Guozhang Wang]

---
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


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



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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


expectedReplicaAssignment seems not used any more.



core/src/test/scala/unit/kafka/admin/AdminTest.scala


Could you add a comment here for bouncing server 1?



core/src/test/scala/unit/kafka/admin/AdminTest.scala


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


Is this println intended?



core/src/test/scala/unit/kafka/admin/DeleteTopicTest.scala


Do we still need expectedReplicaAssignment?



core/src/test/scala/unit/kafka/utils/TestUtils.scala


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

[Jonathan Natkins]

---
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

Review Request 24006: Patch for KAFKA-1420

[Jonathan Natkins]

---
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