----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28016/#review62965 -----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala <https://reviews.apache.org/r/28016/#comment105144> This is quite convoluted code. How about something like this? val changelogPartitions = taskNameToChangeLogPartitionMapping.values.max + 1 (If you're ever tempted to use "var" in Scala, stop and think, because there's probably a better way of doing it.) samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28016/#comment105147> Does the changelog.replication.factor really need to be a top-level config of its own, or could it be under changelog.kafka? It seems odd that the replication factor is singled out like this, whereas other important configs (the compaction policy, the segment size) are not. samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28016/#comment105146> Need documentation for this new set of properties. samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala <https://reviews.apache.org/r/28016/#comment105148> The name you're dealing with here is actually the name of the store, not the changelog. The code would be easier to read if it was clear about this distinction. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala <https://reviews.apache.org/r/28016/#comment105150> As Chris commented previously, it would be better to standardise on "Changelog" rather than "ChangeLog". There are also lots of local variables with [cC]hangeLog in their name, but a case class is particularly visible. samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala <https://reviews.apache.org/r/28016/#comment105145> Are these whitespace changes really necessary? They pollute the diff, and they make the code style inconsistent. (Everywhere else the convention seems to be to align the first asterisks on the same column.) samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala <https://reviews.apache.org/r/28016/#comment105151> See previous comment on "Changelog" rather than "ChangeLog" samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemFactory.scala <https://reviews.apache.org/r/28016/#comment105149> You need to use the store name here, not the topic name. (See my comment on JIRA) - Martin Kleppmann On Nov. 20, 2014, 9:56 p.m., Naveen Somasundaram wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28016/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2014, 9:56 p.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/main/scala/org/apache/samza/util/KafkaUtil.scala > d660b91fb7a1029a47d5e083759b8971ad97e617 > > 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 > >
