You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jason Gustafson <ja...@confluent.io> on 2015/06/19 18:48:23 UTC
Review Request 35655: Patch for KAFKA-2271
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-2271
https://issues.apache.org/jira/browse/KAFKA-2271
Repository: kafka
Description
-------
KAFKA-2271; fix minor test bugs
Diffs
-----
core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
Diff: https://reviews.apache.org/r/35655/diff/
Testing
-------
Thanks,
Jason Gustafson
Re: Review Request 35655: Patch for KAFKA-2271
Posted by Jason Gustafson <ja...@confluent.io>.
> On June 21, 2015, 5:49 a.m., Guozhang Wang wrote:
> > I think the main reason is that util.Random.nextString() may include other non-ascii chars and hence its toString may not be well-defined:
> >
> > http://alvinalexander.com/scala/creating-random-strings-in-scala
> >
> > So I think bottom-line is that we should not use util.Random.nextString to generate random strings. There are a couple of other places where it it used and I suggest we remove them as well.
Since Java uses utf-16 internally, nextString is virtually guaranteed to be non-ascii. But the problem was actually the additional space which was being trimmed. Nevertheless, we might want to avoid using non-ascii characters in assertions unles we can find a reasonable way to display them in test results. The other usages seem legitimate though. One of them explicitly expects non-ascii strings (ApiUtilsTest.testShortStringNonASCII), and the others just use them as arbitrary data of a certain length (OffsetCommitTest.testLargeMetadataPayload).
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/#review88690
-----------------------------------------------------------
On June 19, 2015, 4:48 p.m., Jason Gustafson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35655/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 4:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2271
> https://issues.apache.org/jira/browse/KAFKA-2271
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2271; fix minor test bugs
>
>
> Diffs
> -----
>
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/35655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Gustafson
>
>
Re: Review Request 35655: Patch for KAFKA-2271
Posted by Guozhang Wang <wa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/#review88690
-----------------------------------------------------------
I think the main reason is that util.Random.nextString() may include other non-ascii chars and hence its toString may not be well-defined:
http://alvinalexander.com/scala/creating-random-strings-in-scala
So I think bottom-line is that we should not use util.Random.nextString to generate random strings. There are a couple of other places where it it used and I suggest we remove them as well.
- Guozhang Wang
On June 19, 2015, 4:48 p.m., Jason Gustafson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35655/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 4:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2271
> https://issues.apache.org/jira/browse/KAFKA-2271
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2271; fix minor test bugs
>
>
> Diffs
> -----
>
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/35655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Gustafson
>
>
Re: Review Request 35655: Patch for KAFKA-2271
Posted by Ewen Cheslack-Postava <me...@ewencp.org>.
> On June 22, 2015, 11:43 p.m., Ewen Cheslack-Postava wrote:
> > LGTM. There were 9 question marks when 10 characters were requested, so the problem was probably just that a whitespace character at the start or end would get trimmed during AbstractConfig's parsing.
>
> Jason Gustafson wrote:
> That's what I thought as well, but I was puzzled that I couldn't reproduce it. In fact, it looks like the issue was fixed with KAFKA-2249, which preserves the original properties that were used to construct the config. In that case, however, the assertion basically becomes a tautology, so perhaps we should just remove the test case?
That makes sense. I agree that this test doesn't seem all that useful anymore. I think all the old config classes had tests since they were all written manually. Since the configs are now defined more declaratively via AbstractConfig, these types of tests seem a lot less relevant. It doesn't look like we've generated tests for any other uses of AbstractConfig. Even other tests in that same class are just sort of redundant, e.g. testFromPropsInvalid. It is, I suppose checking that the type is set correctly in the ConfigDef, but mostly it's just retesting the common parsing functionality repeatedly.
- Ewen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/#review88877
-----------------------------------------------------------
On June 19, 2015, 4:48 p.m., Jason Gustafson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35655/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 4:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2271
> https://issues.apache.org/jira/browse/KAFKA-2271
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2271; fix minor test bugs
>
>
> Diffs
> -----
>
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/35655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Gustafson
>
>
Re: Review Request 35655: Patch for KAFKA-2271
Posted by Jason Gustafson <ja...@confluent.io>.
> On June 22, 2015, 11:43 p.m., Ewen Cheslack-Postava wrote:
> > LGTM. There were 9 question marks when 10 characters were requested, so the problem was probably just that a whitespace character at the start or end would get trimmed during AbstractConfig's parsing.
That's what I thought as well, but I was puzzled that I couldn't reproduce it. In fact, it looks like the issue was fixed with KAFKA-2249, which preserves the original properties that were used to construct the config. In that case, however, the assertion basically becomes a tautology, so perhaps we should just remove the test case?
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/#review88877
-----------------------------------------------------------
On June 19, 2015, 4:48 p.m., Jason Gustafson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35655/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 4:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2271
> https://issues.apache.org/jira/browse/KAFKA-2271
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2271; fix minor test bugs
>
>
> Diffs
> -----
>
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/35655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Gustafson
>
>
Re: Review Request 35655: Patch for KAFKA-2271
Posted by Ewen Cheslack-Postava <me...@ewencp.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35655/#review88877
-----------------------------------------------------------
Ship it!
LGTM. There were 9 question marks when 10 characters were requested, so the problem was probably just that a whitespace character at the start or end would get trimmed during AbstractConfig's parsing.
- Ewen Cheslack-Postava
On June 19, 2015, 4:48 p.m., Jason Gustafson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35655/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 4:48 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2271
> https://issues.apache.org/jira/browse/KAFKA-2271
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-2271; fix minor test bugs
>
>
> Diffs
> -----
>
> core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala 98a5b042a710d3c1064b0379db1d152efc9eabee
>
> Diff: https://reviews.apache.org/r/35655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Gustafson
>
>