----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19481/#review38937 -----------------------------------------------------------
Overall, looks really good. Few minor nits, and some build.gradle cleanup. Also: 1. Could you add docs to the website on how to use this tool? 2. Could you write some unit tests for CheckpointTool? build.gradle <https://reviews.apache.org/r/19481/#comment71276> No need to make samza-shell depend on these for runtime. These instructions sound better: http://forums.gradle.org/gradle/topics/how_to_use_in_gradle_javaexec_with_classpath_dependency In short: configurations { gradleShell } dependencies { gradleShell project(":samza-core_$scalaVersion") gradleShell project(":samza-kafka_$scalaVersion") gradleShell project(":samza-yarn_$scalaVersion") gradleShell "org.slf4j:slf4j-simple:$slf4jVersion" } .... classpath = configurations.gradleShell Will need to add slf4jVersion to gradle/dependency-versions.gradle.. also should update the sfl4j testRuntime configuration in samza-kafka to use slf4jVersion. build.gradle <https://reviews.apache.org/r/19481/#comment71283> classpath = configurations.gradleShell build.gradle <https://reviews.apache.org/r/19481/#comment71284> classpath = configurations.gradleShell samza-core/src/main/resources/samza-cmdline-log4j.properties <https://reviews.apache.org/r/19481/#comment71280> This file isn't needed if you use slf4j-simple as described above. I have verified by removing the file and running with: $ ./gradlew checkpointTool The TaskContainer.add() method has been deprecated and is scheduled to be removed in Gradle 2.0. Please use the create() method instead. :samza-api:compileJava UP-TO-DATE :samza-api:processResources UP-TO-DATE :samza-api:classes UP-TO-DATE :samza-api:jar UP-TO-DATE :samza-core_2.10:compileJava UP-TO-DATE :samza-core_2.10:compileScala UP-TO-DATE :samza-core_2.10:processResources UP-TO-DATE :samza-core_2.10:classes UP-TO-DATE :samza-core_2.10:jar UP-TO-DATE :samza-serializers_2.10:compileJava UP-TO-DATE :samza-serializers_2.10:compileScala UP-TO-DATE :samza-serializers_2.10:processResources UP-TO-DATE :samza-serializers_2.10:classes UP-TO-DATE :samza-serializers_2.10:jar UP-TO-DATE :samza-kafka_2.10:compileJava UP-TO-DATE :samza-kafka_2.10:compileScala UP-TO-DATE :samza-kafka_2.10:processResources UP-TO-DATE :samza-kafka_2.10:classes UP-TO-DATE :samza-kafka_2.10:jar UP-TO-DATE :samza-yarn_2.10:compileJava UP-TO-DATE :samza-yarn_2.10:compileScala UP-TO-DATE :samza-yarn_2.10:processResources UP-TO-DATE :samza-yarn_2.10:classes UP-TO-DATE :samza-yarn_2.10:jar UP-TO-DATE :samza-shell:checkpointTool 0 [main] INFO org.apache.samza.checkpoint.CheckpointTool$CommandLine - HAIII The INFO line is just a line I added to verify, since no logging is used in CheckpointTool right now. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71293> remove spaces between = here, just to avoid confusion. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71306> nit pick: can we call this CheckpointToolCommandLine or something, just to avoid two classes with the same name, but different package spaces? samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71282> Should do extends Logging. I prefer logging over println here because we will be using this guy programmatically as well as via the CLI (e.g. to embed in a Web UI). samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71309> Should call manager.register for each partition before calling start. This is the agreement that CheckpointManager defines. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71281> Should do info() here. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment71288> info samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManager.scala <https://reviews.apache.org/r/19481/#comment71292> This might lead to a conflict. I think Ziejie just committed a patch that renames stateTopic to checkpointTopic. Just a heads up. - Chris Riccomini On March 21, 2014, 3:14 p.m., Martin Kleppmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19481/ > ----------------------------------------------------------- > > (Updated March 21, 2014, 3:14 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Make CheckpointTool non-Kafka-specific; move CommandLine to util; move Gradle > task to kafka-shell > > > SAMZA-180: Command-line tool for manipulating checkpoints > > > Diffs > ----- > > build.gradle 8e369b83b7c4a658e1a3660efc92a24efadc9fc1 > samza-core/src/main/resources/samza-cmdline-log4j.properties PRE-CREATION > samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > f3a75afa96a8dc64c98f37fdb88c63075ac2374b > samza-core/src/main/scala/org/apache/samza/util/CommandLine.scala > PRE-CREATION > > samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManager.scala > 27b38b25dc6c34f3ef76d400370d1c857834e6a2 > samza-shell/src/main/bash/checkpoint-tool.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/19481/diff/ > > > Testing > ------- > > > Thanks, > > Martin Kleppmann > >
