----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31590/#review80526 -----------------------------------------------------------
Thanks for the patch. A few comments below. Also, yes, LogConfigTest should be in unit/kafka/log instead of kafka/log. Could you fix it in this patch too? core/src/main/scala/kafka/log/LogManager.scala <https://reviews.apache.org/r/31590/#comment130516> See the coding convention comment below. core/src/main/scala/kafka/log/LogManager.scala <https://reviews.apache.org/r/31590/#comment130504> Our current coding convention is not to wrap single line statement with {}. core/src/main/scala/kafka/server/KafkaConfig.scala <https://reviews.apache.org/r/31590/#comment130525> The original code is more idiomatic in scala. low. - Jun Rao On March 10, 2015, 4:58 a.m., Jeff Holoman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31590/ > ----------------------------------------------------------- > > (Updated March 10, 2015, 4:58 a.m.) > > > Review request for kafka and Gwen Shapira. > > > Bugs: KAFKA-1990 > https://issues.apache.org/jira/browse/KAFKA-1990 > > > Repository: kafka > > > Description > ------- > > KAFKA-1990 > > > Diffs > ----- > > core/src/main/scala/kafka/log/LogConfig.scala > 8b67aee3a37765178b30d79e9e7bb882bdc89c29 > core/src/main/scala/kafka/log/LogManager.scala > 47d250af62c1aa53d11204a332d0684fb4217c8d > core/src/main/scala/kafka/server/KafkaConfig.scala > 48e33626695ad8a28b0018362ac225f11df94973 > core/src/test/scala/kafka/log/LogConfigTest.scala > 9690f141be75202973085025444b52208ebd5ab2 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > ee0b21e6a94ad79c11dd08f6e5adf98c333e2ec9 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 7f47e6f9a74314ed9e9f19d0c97931f3f2e49259 > > Diff: https://reviews.apache.org/r/31590/diff/ > > > Testing > ------- > > Updated for ConfigDef. > > Open question. Is there any reason that LogConfigTest is located in kafka/log > rather than unit/kafka/log ? > > > Thanks, > > Jeff Holoman > >
