> On Nov. 7, 2014, 5 a.m., Gwen Shapira wrote: > > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala, > > line 144 > > <https://reviews.apache.org/r/27634/diff/3/?file=751668#file751668line144> > > > > String.valueOf(false) should evaluate to "false", right? Why do we need > > "nottrue"? > > > > If String.valueOf(false) no longer works as expected, it looks like a > > bug waiting to happen... > > Dmytro Kostiuchenko wrote: > Before my changes boolean values were obtained as StringOps.toBoolean(). > Method throws IllegalArgumentException if called on anything other than > "true" or "false". > Now parsing is handled by ConfigDef. It performs call to > Boolean.parseBoolean(String) which returns false for anything but "true" > throwing no exception. > I updated test to indicate that fact.
Got it. This changes the behavior of "unclean leader election" property into something less safe (typo in "true" can lead to false value for example). Perhaps change the type to "STRING" (similar to how we handle CleanupPolicy) and add a validation to enforce the old behavior? - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27634/#review60188 ----------------------------------------------------------- On Nov. 7, 2014, 1:28 p.m., Dmytro Kostiuchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27634/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2014, 1:28 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1667 > https://issues.apache.org/jira/browse/KAFKA-1667 > > > Repository: kafka > > > Description > ------- > > KAFKA-1667 Fixed bugs in LogConfig. Added test and documentation > > > KAFKA-1667 Updated tests to reflect new boolean property parsing logic > > > KAFKA-1677 renamed methods to match naming convention > > > KAFKA-1667 Added unit test to cover invalid configuration case > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java > c4cea2cc072f4db4ce014b63d226431d3766bef1 > core/src/main/scala/kafka/admin/TopicCommand.scala > 0b2735e7fc42ef9894bef1997b1f06a8ebee5439 > core/src/main/scala/kafka/log/LogConfig.scala > e48922a97727dd0b98f3ae630ebb0af3bef2373d > core/src/main/scala/kafka/utils/Utils.scala > 23aefb4715b177feae1d2f83e8b910653ea10c5f > core/src/test/scala/kafka/log/LogConfigTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala > f44568cb25edf25db857415119018fd4c9922f61 > > Diff: https://reviews.apache.org/r/27634/diff/ > > > Testing > ------- > > > Thanks, > > Dmytro Kostiuchenko > >
