----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28687/#review63917 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/common/config/ConfigDef.java <https://reviews.apache.org/r/28687/#comment106244> Seems like this might be better off going into samza-api. That way modules could pull it in without even pulling in core. Also, there are no dependencies on it other than ConfigException, which is also in api. samza-core/src/main/java/org/apache/samza/common/config/ConfigDef.java <https://reviews.apache.org/r/28687/#comment106245> Reformat to Samza Apache 2.0 line width. samza-core/src/main/java/org/apache/samza/common/config/ConfigDef.java <https://reviews.apache.org/r/28687/#comment106247> Are there any tests for this class that we should also suck in from Kafka? samza-core/src/main/java/org/apache/samza/common/config/ConfigDef.java <https://reviews.apache.org/r/28687/#comment106246> Reformat all to use 2 space tabs rather than 4 space. samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28687/#comment106256> Nit: space before Map samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28687/#comment106255> No need for : Unit = samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28687/#comment106249> Shouldn't this be .toInt instead of asInstanceOf[Int]? samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28687/#comment106251> Brainstorm: it would be cool if we could have some method that would combine getOption and getOrElse. It could get the value. If it doens't exist, it could throw the default exception, and maybe spew the docs from the ConfigDef for the missing property as well. samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28687/#comment106254> Seems like we could just have zkConnect: String samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfigUtils.scala <https://reviews.apache.org/r/28687/#comment106252> Apache license. samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfigUtils.scala <https://reviews.apache.org/r/28687/#comment106253> Seems like this couldgo in samza-kafka/src/test/... - Chris Riccomini On Dec. 4, 2014, 2:01 a.m., Chinmay Soman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28687/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2014, 2:01 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > This is just part of SAMZA-471. I'm submitting this RB to get feedback on > this new way of defining and maintaining config. This RB attempts to simplify > config for the samza-kafka module. The changes are summarized below: > * Copied ConfigDef from the Kafka repo - this is used within KafkaConfig to > define defaults and automatic documentation. > * All the getOrElse have been moved to the config > > Open questions: > ================ > * How to manage the config with replaceable term in it (For eg: > "job.config.rewriter.%s.regex") using ConfigDef ? > > > Diffs > ----- > > build.gradle 38383bd9e3f0847d6088a4ea4c1ee6f3dcd1e430 > samza-core/src/main/java/org/apache/samza/common/config/ConfigDef.java > PRE-CREATION > > samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala > f7db2a1fc5217343bf808e3dcf20315822c1bcde > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala > 9fc1f56d4404ec7722c0d34fde2804e981b41309 > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfigUtils.scala > PRE-CREATION > samza-kafka/src/main/scala/org/apache/samza/config/KafkaSerdeConfig.scala > 11078e3a3fa3323140a630a55c2fc4db345a8168 > > samza-kafka/src/main/scala/org/apache/samza/config/RegExTopicGenerator.scala > a34c3f2738855dbf3737639c33846fcad23bd3b9 > samza-kafka/src/main/scala/org/apache/samza/serializers/KafkaSerde.scala > 82ba2a09b98a04ac64301743b3ae32f29cefbc3b > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala > 4ed5e881031e019d8df6de259cabb658820a3ba0 > samza-kafka/src/main/scala/org/apache/samza/util/KafkaUtil.scala > d660b91fb7a1029a47d5e083759b8971ad97e617 > > samza-kafka/src/test/scala/org/apache/samza/checkpoint/kafka/TestKafkaCheckpointManager.scala > 553d6b4d6ffe21f4a92c8c347e835d95d71b5863 > samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaConfig.scala > 8109f73a66adea671743f0212312cfa53b094311 > > samza-kafka/src/test/scala/org/apache/samza/config/TestKafkaSerdeConfig.scala > 5cf82c2f95b5e7bfa7d73efebeafaea8d6b09023 > > samza-kafka/src/test/scala/org/apache/samza/config/TestRegExTopicGenerator.scala > 89ced3421f0ba1b6de75f7d33d425736f6c970ec > > samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemFactory.scala > 8067cbf78214d3c01b7f915d8810b10de57fe6a3 > > Diff: https://reviews.apache.org/r/28687/diff/ > > > Testing > ------- > > Added more unit tests > > ./gradlew clean test (passes) > > > Thanks, > > Chinmay Soman > >
