> 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...
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. > On Nov. 7, 2014, 5 a.m., Gwen Shapira wrote: > > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala, > > lines 151-161 > > <https://reviews.apache.org/r/27634/diff/3/?file=751668#file751668line151> > > > > why are we removing a test? As I said, parser no longer throws IAE, so assigning any invalid value to that property will be considered as false. Thus the whole case is no longer valid. > On Nov. 7, 2014, 5 a.m., Gwen Shapira wrote: > > core/src/main/scala/kafka/utils/Utils.scala, lines 514-522 > > <https://reviews.apache.org/r/27634/diff/3/?file=751666#file751666line514> > > > > I think I'm not seeing why we need this. > > Shouldn't Scala's JavaConversion class handle this exact case? Frankly, I'm new to Scala. But I didn't found a way to convert java.util.Properties to Map respecting defaults. java.util.Properties have additional method getProperty() which checks for value in nested Properties object (see Properties(Properties defaults) constructor) if key is absent in this one. But is treated as Map, defaults can't be accessed. Map.get(K, V) is implemented in Properties' parent — HashTable, and thus, get() knows nothing about default properties. Example: val defaults = new Properties() defaults.put("foo", "bar") val props = new Properties(defaults) props.get("foo") // null props.getProperty("foo") // "bar" > On Nov. 7, 2014, 5 a.m., Gwen Shapira wrote: > > core/src/test/scala/kafka/log/LogConfigTest.scala, line 29 > > <https://reviews.apache.org/r/27634/diff/3/?file=751667#file751667line29> > > > > Can you add a test that shows that we indeed fail validation when > > creating LogConfig with invalid values? Will do that. Do you think creating testcase for every property manually would be a good idea? I can't come up right now with anything, that would simplify generating testcases and would reduce maintenance cost. - Dmytro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27634/#review60188 ----------------------------------------------------------- On Nov. 6, 2014, 4:12 p.m., Dmytro Kostiuchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27634/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2014, 4:12 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-1667 renamed methods to match naming convention > > > 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 > >
