----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27634/#review59997 -----------------------------------------------------------
I like how you added validations while preserving the API to minimize code changes. However, looks like not all unit tests pass. For example, DynamicConfigChangeTest is failing. Can you run the tests (./gradlew cleanTest test) and make sure they all pass? Also, I recommend adding extra tests to test the validation. core/src/main/scala/kafka/log/LogConfig.scala <https://reviews.apache.org/r/27634/#comment101339> I think the validations look good, so you can remove the TODO core/src/main/scala/kafka/log/LogConfig.scala <https://reviews.apache.org/r/27634/#comment101337> I'd copy-paste the docs for each property either from the javadoc for the class or from the documentation and add them. - Gwen Shapira On Nov. 5, 2014, 6:44 p.m., Dmytro Kostiuchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27634/ > ----------------------------------------------------------- > > (Updated Nov. 5, 2014, 6:44 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1667 > https://issues.apache.org/jira/browse/KAFKA-1667 > > > Repository: kafka > > > Description > ------- > > KAFKA-1667 Added topic level config validation > > > 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 > > Diff: https://reviews.apache.org/r/27634/diff/ > > > Testing > ------- > > > Thanks, > > Dmytro Kostiuchenko > >
