> On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 53 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line53> > > > > Not needed.
removed > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 65 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line65> > > > > Prefer = Map() over null. done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 90 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line90> > > > > No need for :Unit = removed > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 91 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line91> > > > > Remove newlines. removed > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 96 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line96> > > > > Message in SamzaException. > > > > space after , done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala, > > line 98 > > <https://reviews.apache.org/r/28016/diff/1/?file=763002#file763002line98> > > > > Indentation. done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala, line > > 37 > > <https://reviews.apache.org/r/28016/diff/1/?file=763005#file763005line37> > > > > I don't think we need this, do we? Is there ever a case where you'd > > want to set the changelog partition count to a non-default number? removed as per previous comment > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala, line > > 98 > > <https://reviews.apache.org/r/28016/diff/1/?file=763005#file763005line98> > > > > map -> foreach changed > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 90 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line90> > > > > Javadocs. > > > > After seeing the implementation, I prefer not passing a config object > > into the constructor, and just having a constructor with a lot of defaults > > (as we had before). The reason that I don't like this is: > > > > 1. It's impossible to know what to set in the config to create the > > KafkaSystemAdmin without reading its code. > > 2. It bleeds wiring with the class itself. These are two separate > > concerns, and should be treated separately. > > 3. It makes writing tests harder, since you have to create a config > > object, rather than just passing in the params. > > > > Recommend switching back to the old style, and just adding new params > > to the constructor. switched to passing parameters > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 283 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line283> > > > > This code looks to be pretty much copied from KafkaCheckpointManager. > > Can we just have the code once, and call it in both places? Maybe in > > KafkaUtil? As discussed, there are different parameters and validations for checkpoint and changelog stream creation. I'm going to keep them separate, as I expect to diverge further. > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 286 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line286> > > > > Don't think we need this. Should just use numKafkaChangeLogPartitions. > > > > Also, we should try and standardize on "Changelog" not "ChangeLog". Removed it as addressed in previous comment. > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 303 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line303> > > > > checkpoint topic -> changelog topic done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 310 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line310> > > > > Checkpoint -> changelog. done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 333 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line333> > > > > Checkpoint again. done > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-kafka/src/main/scala/org/apache/samza/system/kafka/KafkaSystemAdmin.scala, > > line 351 > > <https://reviews.apache.org/r/28016/diff/1/?file=763006#file763006line351> > > > > What is this? Javadocs. added docs > On Nov. 14, 2014, 5:32 p.m., Chris Riccomini wrote: > > samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala, > > line 216 > > <https://reviews.apache.org/r/28016/diff/1/?file=763010#file763010line216> > > > > Remove. done - Naveen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28016/#review61445 ----------------------------------------------------------- On Nov. 19, 2014, 1:19 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, 1:19 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 > >
