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 28016: SAMZA-226: Auto create Changelog stream topics
Date Wed, 19 Nov 2014 18:23:39 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28016/#review62174
-----------------------------------------------------------



.reviewboardrc
<https://reviews.apache.org/r/28016/#comment104151>

    OK for 0.8.0 branch. Will need to remove this for master.



.reviewboardrc
<https://reviews.apache.org/r/28016/#comment104153>

    OK for 0.8.0 branch, but will need to fix in patch for master.



docs/learn/documentation/versioned/jobs/configuration-table.html
<https://reviews.apache.org/r/28016/#comment104152>

    Nit: trailing space.



docs/learn/documentation/versioned/jobs/configuration-table.html
<https://reviews.apache.org/r/28016/#comment104154>

    Nit: trailing space.



samza-api/src/main/java/org/apache/samza/config/Config.java
<https://reviews.apache.org/r/28016/#comment104150>

    Nit: regexSubset (lowercase S) to match "subset" API.



samza-api/src/main/java/org/apache/samza/config/Config.java
<https://reviews.apache.org/r/28016/#comment104155>

    regexSubset (lower case s) to match "subset" API (vs. subSet).



samza-api/src/main/java/org/apache/samza/config/Config.java
<https://reviews.apache.org/r/28016/#comment104156>

    Nit: bracket should be on the same line as if().



samza-api/src/main/java/org/apache/samza/config/Config.java
<https://reviews.apache.org/r/28016/#comment104157>

    Nit: k, entry (space)



samza-api/src/main/java/org/apache/samza/system/SystemAdmin.java
<https://reviews.apache.org/r/28016/#comment104158>

    Nit: (for e.g. ...) seems redundant. You already said that the method is an API to create
change log streams.
    
    Remove "topicName" and use "streamName" instead. We don't use "topic" in Samza.
    
    Nit: Also, you have both changelog and "change log" in the same docs. Please use "changelog".
Trying to get standardized on that. :)



samza-api/src/main/java/org/apache/samza/util/SinglePartitionWithoutOffsetsSystemAdmin.java
<https://reviews.apache.org/r/28016/#comment104160>

    Spacing is all over the place here. Please clean up.



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/28016/#comment104162>

    It seems like this stuff needs to be calculated only once, not once per-task. Recommend
moving outside the taskInstances loop.



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/28016/#comment104161>

    Comment why we're doing this.



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/28016/#comment104164>

    Nit: Named parameter seems not necessary. Recommend removing "partition = " if possible.



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/28016/#comment104163>

    Nit: Named parameter seems not necessary. Recommend removing "partition = " if possible.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104165>

    Not used.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104166>

    Not used.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104168>

    Nit: streamMetadataCache: StreamMetadataCache vs. streamMetadataCache : StreamMetadataCache
(eliminate space to match style)



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104169>

    Nit: eliminate space to match style



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104170>

    This log line causes a compie-time error. You also have %s, but never call format.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala
<https://reviews.apache.org/r/28016/#comment104171>

    Nit: private



samza-core/src/main/scala/org/apache/samza/system/filereader/FileReaderSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104172>

    Nit: single space.



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104177>

    Nit: ChangeLog -> Changelog



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104179>

    Recommend ArrayBuffer here. Then, buffer.toList at the end.



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104180>

    This logic won't work. You're looking at the prefix of the SystemStream, and assuming
that developers will always name their system "kafka". This isn't necessarily true. They could
name their system "my-system", and it could still be a Kafka system.



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104175>

    Style: single-line braces.. for(...) {



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104176>

    Style: single-line braces.. if(...) {



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104173>

    Nit: ... name, true (space)



samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala
<https://reviews.apache.org/r/28016/#comment104174>

    Nit: key, value (space)



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104181>

    Javadocs.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104184>

    Nit: newline.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104182>

    Prefer () => ZkClient as a parameter here (see KafkaCheckpointManager for an example).
This makes the ZkClient injectabe for unit testing.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104183>

    Should just be Map[String, ChangeLogInfo]



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104190>

    To make this a little more clear, recommend "Using partition count " + ...



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104191>

    Style: kafkaProps)



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104189>

    Move to KafkaUtil and share with KafkaCheckpointManager.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104187>

    Delete or define.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104188>

    Delete or define.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104185>

    No need for : Unit =.. Just do ) {



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala
<https://reviews.apache.org/r/28016/#comment104186>

    Nit: newline.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala
<https://reviews.apache.org/r/28016/#comment104192>

    No need to have var and mutable map. Should be either/or. Stylistically, we use var with
immutable map for everywhere except performance critical code. So, var ... Map()



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala
<https://reviews.apache.org/r/28016/#comment104193>

    Style: single line for {



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala
<https://reviews.apache.org/r/28016/#comment104195>

    Clean this up and and make multi-line, if possible.
    
    Recommend "2" vs 2.toString (to be succinct).
    
    Recommend creating the changeLogInfo in one line, and .put() in another.
    
    Recommend getting replication factor in one line and using it in ChangeogInfo in another.



samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala
<https://reviews.apache.org/r/28016/#comment104194>

    This really confused me. Should be consumerConfig (lower case C). Thought you were referring
to a class below when you referenced ConsumerConfig.zkConnect.


- Chris Riccomini


On Nov. 19, 2014, 2:12 a.m., Naveen Somasundaram wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28016/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2014, 2:12 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> I have added an new method to the system admin as discussed in the jira, the task storage
manager fetches all the information necessary and creates the change log topic using the system
admin.
> 
> PENDING: I have to update the Samza docs with the new configurations added, will update
the rb with docs updates
> 
> 
> Diffs
> -----
> 
>   .reviewboardrc 9339119e248e41f954d47e1d01a0f2e1130d349c 
>   docs/learn/documentation/versioned/jobs/configuration-table.html 4266a137ae003e946e11c122d94061c31d643c77

>   samza-api/src/main/java/org/apache/samza/config/Config.java 2048e90e80e21086eb59be57f3bcd5ebf92b2811

>   samza-api/src/main/java/org/apache/samza/system/SystemAdmin.java 571c60631357ea9a0b4fa24e7253008619ef2f32

>   samza-api/src/main/java/org/apache/samza/util/SinglePartitionWithoutOffsetsSystemAdmin.java
38e313f3c39454110efd354e6ca025869fa930cd 
>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala d91d6d7940bd07a145dd3b782a9239f24bb5cf2e

>   samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala b8719c36c2b9346bcd3f291e23b33d2c00cebfa9

>   samza-core/src/main/scala/org/apache/samza/system/filereader/FileReaderSystemAdmin.scala
98e92bc12f3e2827cdec02f1ce94d7e2314e4b4e 
>   samza-core/src/test/scala/org/apache/samza/checkpoint/TestOffsetManager.scala a79eccaa8fc18d197b77f9363f1814fefc4ac40d

>   samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 9fc1f56d4404ec7722c0d34fde2804e981b41309

>   samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala 5ac33ea36da451250655d9dd373692b964322b41

>   samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala 4ed5e881031e019d8df6de259cabb658820a3ba0

>   samza-kafka/src/test/scala/org/apache/samza/system/kafka/TestKafkaSystemAdmin.scala
5ceb1093a66cb57e298d4b3ccdd24845dbb41b58 
>   samza-test/src/main/java/org/apache/samza/system/mock/MockSystemAdmin.java fa1d51b290013a3913d64884dc43907a76670849

>   samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala
118f5eee22016db3b802c32fb26c5d72fa61f1a7 
> 
> Diff: https://reviews.apache.org/r/28016/diff/
> 
> 
> Testing
> -------
> 
> Modoified TestStatefulTask to disable auto creation of topics and the test seems to work.
> 
> 
> Thanks,
> 
> Naveen Somasundaram
> 
>


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