> 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. > > Gwen Shapira wrote: > 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. > > Jun Rao wrote: > Got it. Should we use the constants defined in KafkaConfig to void human > errors?
+1 on using the constants! - Onur ----------------------------------------------------------- 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 > >
