> On April 8, 2015, 4:51 a.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala, line 137
> > <https://reviews.apache.org/r/32953/diff/1/?file=920411#file920411line137>
> >
> >     Do you know why the test passes before? ConfigDef currently seems to 
> > ignore undefined properties, instead of throwing an exception.
> 
> Onur Karaman wrote:
>     KafkaConfig's configDef has:
>     ```
>     .define(AdvertisedListenersProp, STRING, HIGH, AdvertisedListenersDoc, 
> false)
>     ```
>     
>     So AdvertisedListenersProp doesn't require a default and no default is 
> provided.
>     With the typo, no required default, and no provided default, 
> ConfigDef.parse just sets the value associated with "advertised.listeners" to 
> null.

Actually, this is not why the test passed.

The test checks for *invalid* configuration - i.e. we set the configuration and 
pass if IllegalArgumentException is thrown.
In this case, the exception will be thrown if the configuration is invalid 
because we set the same port twice (what we wanted to test) or if the 
configuration is invalid because we mistakenly configured a non-existing 
parameter (what we actually tested).

I'm not sure how we want to address this (or if it is even worth addressing) - 
having different exceptions for different types of invalid configurations seems 
a too confusing.
We can change the test to check the specific text of the exception, but I 
really wouldn't want changes of messages to break tests.


- Gwen


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


On April 8, 2015, 1:54 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32953/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 1:54 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2104
>     https://issues.apache.org/jira/browse/KAFKA-2104
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> fixed typo
> 
> 
> Diffs
> -----
> 
>   core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 
> b3947191735ee149d050bb48db7bc3b2483f2dd7 
> 
> Diff: https://reviews.apache.org/r/32953/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Reply via email to