samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Riccomini" <criccom...@apache.org>
Subject Re: Review Request 28687: SAMZA-471: (part 1) Make config easy to specify default values and validation
Date Thu, 04 Dec 2014 22:26:06 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message