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