You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jeff Holoman <je...@gmail.com> on 2014/12/09 19:11:20 UTC
Review Request 28859: Patch for KAFKA-1812
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28859/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1812
https://issues.apache.org/jira/browse/KAFKA-1812
Repository: kafka
Description
-------
KAFKA-1812 initial
Diffs
-----
core/src/main/scala/kafka/utils/Utils.scala 58685cc47b4c43e4ee68b73f1ee34eb99a5aa547
core/src/test/scala/unit/kafka/utils/UtilsTest.scala 0d0f0e2fba367180eeb718a259e8d680a73c3a73
Diff: https://reviews.apache.org/r/28859/diff/
Testing
-------
Thanks,
Jeff Holoman
Re: Review Request 28859: Patch for KAFKA-1812
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28859/#review64689
-----------------------------------------------------------
Ship it!
Non-binding +1
- Gwen Shapira
On Dec. 11, 2014, 2:39 a.m., Jeff Holoman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28859/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 2:39 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1812
> https://issues.apache.org/jira/browse/KAFKA-1812
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1812 Updated based on rb comments
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/utils/Utils.scala 58685cc47b4c43e4ee68b73f1ee34eb99a5aa547
> core/src/test/scala/unit/kafka/utils/UtilsTest.scala 0d0f0e2fba367180eeb718a259e8d680a73c3a73
>
> Diff: https://reviews.apache.org/r/28859/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jeff Holoman
>
>
Re: Review Request 28859: Patch for KAFKA-1812
Posted by Jeff Holoman <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28859/
-----------------------------------------------------------
(Updated Dec. 11, 2014, 2:39 a.m.)
Review request for kafka.
Bugs: KAFKA-1812
https://issues.apache.org/jira/browse/KAFKA-1812
Repository: kafka
Description (updated)
-------
KAFKA-1812 Updated based on rb comments
Diffs (updated)
-----
core/src/main/scala/kafka/utils/Utils.scala 58685cc47b4c43e4ee68b73f1ee34eb99a5aa547
core/src/test/scala/unit/kafka/utils/UtilsTest.scala 0d0f0e2fba367180eeb718a259e8d680a73c3a73
Diff: https://reviews.apache.org/r/28859/diff/
Testing
-------
Thanks,
Jeff Holoman
Re: Review Request 28859: Patch for KAFKA-1812
Posted by Gwen Shapira <gs...@cloudera.com>.
> On Dec. 10, 2014, 3:21 p.m., Gwen Shapira wrote:
> > Non-binding LGTM from me.
> >
> > Any chance of cleaning up the whitespace? makes the change looks way bigger than it really is.
> >
> > Few suggested improvements below:
My mistake - the whitespaces were in the code originally and this patch cleans them up.
Sorry and thanks for the cleanup :)
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28859/#review64546
-----------------------------------------------------------
On Dec. 9, 2014, 6:11 p.m., Jeff Holoman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28859/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 6:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1812
> https://issues.apache.org/jira/browse/KAFKA-1812
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1812 initial
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/utils/Utils.scala 58685cc47b4c43e4ee68b73f1ee34eb99a5aa547
> core/src/test/scala/unit/kafka/utils/UtilsTest.scala 0d0f0e2fba367180eeb718a259e8d680a73c3a73
>
> Diff: https://reviews.apache.org/r/28859/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jeff Holoman
>
>
Re: Review Request 28859: Patch for KAFKA-1812
Posted by Gwen Shapira <gs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28859/#review64546
-----------------------------------------------------------
Non-binding LGTM from me.
Any chance of cleaning up the whitespace? makes the change looks way bigger than it really is.
Few suggested improvements below:
core/src/main/scala/kafka/utils/Utils.scala
<https://reviews.apache.org/r/28859/#comment107303>
Scala has Pair type. Since we always expect 2 elements, it will be more descriptive and safer to use it instead of the internal Array.
core/src/test/scala/unit/kafka/utils/UtilsTest.scala
<https://reviews.apache.org/r/28859/#comment107304>
Since this is a generic function, I'd add tests for:
1. list with a single host:port pair (i.e. no commas)
2. ipv4
- Gwen Shapira
On Dec. 9, 2014, 6:11 p.m., Jeff Holoman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28859/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2014, 6:11 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1812
> https://issues.apache.org/jira/browse/KAFKA-1812
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1812 initial
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/utils/Utils.scala 58685cc47b4c43e4ee68b73f1ee34eb99a5aa547
> core/src/test/scala/unit/kafka/utils/UtilsTest.scala 0d0f0e2fba367180eeb718a259e8d680a73c3a73
>
> Diff: https://reviews.apache.org/r/28859/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jeff Holoman
>
>