You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chinmay Soman <ch...@gmail.com> on 2014/12/04 02:53:41 UTC
Review Request 28687: SAMZA-471: (part 1) Make config easy to specify
default values and validation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28687/
-----------------------------------------------------------
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
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
Re: Review Request 28687: SAMZA-471: (part 1) Make config easy to
specify default values and validation
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 28687: SAMZA-471: (part 1) Make config easy to
specify default values and validation
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-------
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