----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27246/#review58703 -----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99812> License needs to be at line 0 (above package. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99813> Use <p></p> to wrap paragraphs, or Javadocs will run everything together in HTML. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99828> Seems to be never used, other than logCheckpoint, which is also never used. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99816> Constants should be in `object CheckpointMigrationTool`, so they can be referenced as static variables. Also, seems like LOG_TYPE is never used. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99814> classOf[GroupByPartitionFactory].getCanonicalName samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99817> Nit: random white space samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99821> Nit: code base has {} on single line if statements. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99826> It seems dangerous to default to "None" if we can't find a checkpoint for the SSP. I think we should throw an exception in such a case. As it is now, getOrElse(None).toString will put something like SSP1 -> "None", which will cause an exception when the job starts, since it'll try to do "None".toInt samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99827> Never used. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99818> Move constants to `object KafkaHelper`, and refernece statically. Also, unclear to me what value LOG_TYPE provides, since it's just being used once in the middle of a log line. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99820> Client ID should probably be something like "samza-checkpoint-migration-tool". samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99819> Would rather re-use KafkaCheckpointManagerFactory's method. We'll have to move it to the `object`, and make it, but seems better than duplicating logic. samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99823> I think this can be replaced by Util.getInputStreamPartitions samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala <https://reviews.apache.org/r/27246/#comment99829> classOf[...].getCanonicalName Also, I want to confirm--you're developing this off of the 0.8.0 branch, not master, right? - Chris Riccomini On Oct. 27, 2014, 9:40 p.m., Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27246/ > ----------------------------------------------------------- > > (Updated Oct. 27, 2014, 9:40 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Adding checkpoint migration tool LISAMZA-594 > > > Diffs > ----- > > build.gradle 828bce9913db00161971607e4c9ac19c63cecb95 > > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala > PRE-CREATION > samza-shell/src/main/bash/run-migration-tool.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/27246/diff/ > > > Testing > ------- > > Created a test job "samza-migration-mp" using 0.7 version. Stopped the job > and ran the migration tool. > > > Thanks, > > Navina Ramesh > >
