----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19481/#review38050 -----------------------------------------------------------
build.gradle <https://reviews.apache.org/r/19481/#comment69998> Should not depend on slf4j-log4j in a library. If you want log4j for the checkpointTool task, I think we can set it in classpath = ... below. Also, for simple task, we should just use slf4j-simple instead of slf4j-log4j, which just writes to STDOUT. This means we can skip the log4j.properties file below, as well. build.gradle <https://reviews.apache.org/r/19481/#comment70000> This is pretty useful. Two thoughts: 1. I think we should move it to samza-shell. 2. We should make two tasks, runJob and writeCheckpoint, so we can run both the JobRunner and the CheckpointTool. samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala <https://reviews.apache.org/r/19481/#comment70007> Move to samza-core's util package. samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala <https://reviews.apache.org/r/19481/#comment70001> Can we combine these two methods (parser.parse, and loadConfig into one method? It seems like we would always use these two methods back-to-back for every CommandLine. Maybe just cmdline.loadConfig(args: _*) samza-kafka/src/main/resources/checkpoint-tool-log4j.properties <https://reviews.apache.org/r/19481/#comment70003> No need for this file if we just use slf4j-simple in the checkpointTool task. samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment70008> This is a great tool, but I think it not need be Kafka-specific. Can you make it generic (the way JobRunner is), and move it into samza-core's checkpoint package? This way, we can use the checkpoint tool whether we use a KafkaCheckpointManager, HDFSCheckpointManager, ZookeeperCheckpointManager, etc. samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/CheckpointTool.scala <https://reviews.apache.org/r/19481/#comment70009> To make the checkpoint tool generic, I think you just need to use the checkpoint manager factory defined for the system in the config. Everything else here appears to be totally generic. samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManager.scala <https://reviews.apache.org/r/19481/#comment70010> Shouldn't need any of these to be vals if you convert the CheckpointTool to support arbitrary CheckpointManagers. samza-shell/src/main/bash/checkpoint-tool.sh <https://reviews.apache.org/r/19481/#comment70006> No need for log4j setting here. run-class.sh will auto-add it if it exists in the lib directory of a package. If it doesn't, it's up to the user to add an appropriate slf4j module (e.g. slf4j-simple, slf4j-avsl, etc), and configure it with SAMZA_OPTS, accordingly. - Chris Riccomini On March 20, 2014, 11:04 p.m., Martin Kleppmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19481/ > ----------------------------------------------------------- > > (Updated March 20, 2014, 11:04 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > SAMZA-180: Command-line tool for manipulating checkpoints > > > Diffs > ----- > > build.gradle 8e369b83b7c4a658e1a3660efc92a24efadc9fc1 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > f3a75afa96a8dc64c98f37fdb88c63075ac2374b > samza-kafka/src/main/resources/checkpoint-tool-log4j.properties > PRE-CREATION > > samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/CheckpointTool.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 > >
