You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Navina Ramesh <nr...@linkedin.com> on 2014/10/27 22:40:09 UTC
Review Request 27246: Adding checkpoint migration tool LISAMZA-594
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/
-----------------------------------------------------------
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
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58810
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99978>
I think we should throw an exception. The only way in which this can be none (as far as I can tell) is if the grouper were mis-behaved, and somehow added SSPs to taskNameToSSP that weren't in the lastCheckpoint set, or removed SSPs that were in lastCheckpoint set. In both cases, this is a bad SSP grouper, and an exception should be thrown.
- Chris Riccomini
On Oct. 28, 2014, 2:06 a.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 2:06 a.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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Chris Riccomini <cr...@apache.org>.
> On Oct. 28, 2014, 9:24 p.m., Chris Riccomini wrote:
> > Ship It!
One nit on the warn(), which will result in a runtime error.
Also, please post the updated patch to JIRA when you've updated. The JIRA post is what grants ASF ownership of the code.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58874
-----------------------------------------------------------
On Oct. 28, 2014, 9:14 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 9:14 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding checkpoint migration tool SAMZA-354
>
>
> Diffs
> -----
>
> build.gradle 64c223a
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
> 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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58874
-----------------------------------------------------------
Ship it!
Ship It!
- Chris Riccomini
On Oct. 28, 2014, 9:14 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 9:14 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding checkpoint migration tool SAMZA-354
>
>
> Diffs
> -----
>
> build.gradle 64c223a
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
> 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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58870
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment100021>
Formatting. Three %s and two parameters.
- Chris Riccomini
On Oct. 28, 2014, 9:14 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 9:14 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding checkpoint migration tool SAMZA-354
>
>
> Diffs
> -----
>
> build.gradle 64c223a
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
> 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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 9:34 p.m.)
Review request for samza.
Changes
-------
Fixed formatting in warn statement
Repository: samza
Description
-------
Adding checkpoint migration tool SAMZA-354
Diffs (updated)
-----
build.gradle 64c223a
samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
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. Upgraded to 0.8 version and restarted the job. Works fine
Thanks,
Navina Ramesh
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 9:14 p.m.)
Review request for samza.
Changes
-------
Made changes based on feedback
Summary (updated)
-----------------
Adding checkpoint migration tool SAMZA-354
Repository: samza
Description (updated)
-------
Adding checkpoint migration tool SAMZA-354
Diffs (updated)
-----
build.gradle 64c223a
samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
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. Upgraded to 0.8 version and restarted the job. Works fine
Thanks,
Navina Ramesh
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58813
-----------------------------------------------------------
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99980>
thinking if it is possible to put some of the methods, such as createTopic to org.apache.samza.util.KafkaUtil , or move the KafkaHelper to a seprate class for reusing by other code, such as SAMZA-226, SAMZA-310. Duplicating code seems awkward for me.
- Yan Fang
On Oct. 28, 2014, 2:06 a.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 2:06 a.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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/
-----------------------------------------------------------
(Updated Oct. 28, 2014, 2:06 a.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 (updated)
-------
Created a test job "samza-migration-mp" using 0.7 version. Stopped the job and ran the migration tool. Upgraded to 0.8 version and restarted the job. Works fine
Thanks,
Navina Ramesh
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Navina Ramesh <nr...@linkedin.com>.
> On Oct. 28, 2014, 1:44 a.m., Chinmay Soman wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 102
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line102>
> >
> > Do you also want to check if topic exists for oldCheckpointTopic ?
Since this tool is applicable only for migration scenario, I assumed that if the new checkpoint topic exists, then old format checkpoint topic probably existed. Do you forsee a scenario where this tool is used when there is no old format checkpoint topic?
> On Oct. 28, 2014, 1:44 a.m., Chinmay Soman wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 97
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line97>
> >
> > Same here
Nice catch. will fix it.
> On Oct. 28, 2014, 1:44 a.m., Chinmay Soman wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 267
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line267>
> >
> > This method looks pretty big to me. Not strict - but if you could break it up into smaller ones - that'll be nice.
I tried to. But the interfaces in 0.7 is kind of tightly coupled with the config. Do you see any value in doing so?
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58746
-----------------------------------------------------------
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
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58746
-----------------------------------------------------------
Ship it!
Looks good overall ! Left some minor comments.
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99885>
Missing format args
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99886>
Same here
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99890>
Do you also want to check if topic exists for oldCheckpointTopic ?
samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala
<https://reviews.apache.org/r/27246/#comment99893>
This method looks pretty big to me. Not strict - but if you could break it up into smaller ones - that'll be nice.
- Chinmay Soman
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
>
>
Re: Review Request 27246: Adding checkpoint migration tool SAMZA-354
Posted by Navina Ramesh <nr...@linkedin.com>.
On Oct. 27, 2014, 11 p.m., Navina Ramesh wrote:
> > Also, I want to confirm--you're developing this off of the 0.8.0 branch, not master, right?
>
> Navina Ramesh wrote:
> I am developing off master branch :( Should I develop this off the 0.8.0 branch ?
>
> Chris Riccomini wrote:
> Yeah, develop off of 0.8. The reason I say that is that 0.9 (master) is currently very turbulent due to my changes, and things like TaskNameToSystemStreamPartition are gone in master.
>
> Since this tool is to migrate from 0.7 to 0.8, I think we should just include it in the 0.8 branch (not master).
>
> Chris Riccomini wrote:
> Also, regarding testing, did you validate that the offsets were properly restored after the migration? It should be shown in the logs.
Diff #2 is based off 0.8.
Yes. I validated the offsets in the new topic for the hello-world job. I also upgraded the test job to 0.8 and deployed. It worked fine.
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58703
-----------------------------------------------------------
On Oct. 28, 2014, 9:14 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 9:14 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding checkpoint migration tool SAMZA-354
>
>
> Diffs
> -----
>
> build.gradle 64c223a
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala f7db2a1
> 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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Chris Riccomini <cr...@apache.org>.
On Oct. 27, 2014, 11 p.m., Navina Ramesh wrote:
> > Also, I want to confirm--you're developing this off of the 0.8.0 branch, not master, right?
>
> Navina Ramesh wrote:
> I am developing off master branch :( Should I develop this off the 0.8.0 branch ?
Yeah, develop off of 0.8. The reason I say that is that 0.9 (master) is currently very turbulent due to my changes, and things like TaskNameToSystemStreamPartition are gone in master.
Since this tool is to migrate from 0.7 to 0.8, I think we should just include it in the 0.8 branch (not master).
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58703
-----------------------------------------------------------
On Oct. 28, 2014, 2:06 a.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 2:06 a.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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Navina Ramesh <nr...@linkedin.com>.
> On Oct. 27, 2014, 11 p.m., Chris Riccomini wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 191
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line191>
> >
> > 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.
Yeah. I didn't move it to object because it was defined as "private" and I didn't understand why. I will move it to object as it makes more sense.
> On Oct. 27, 2014, 11 p.m., Chris Riccomini wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 131
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line131>
> >
> > 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
I wasn't sure how to handle this actually. A good way for the tool to handle the "null" scenario can be to read the consumer offset.reset settings. I am not sure if we should go this route or just throw an exception and let the user deal with fixing it.
> On Oct. 27, 2014, 11 p.m., Chris Riccomini wrote:
> > samza-kafka/src/main/scala/org/apache/samza/util/CheckpointMigrationTool.scala, line 67
> > <https://reviews.apache.org/r/27246/diff/1/?file=734567#file734567line67>
> >
> > Seems to be never used, other than logCheckpoint, which is also never used.
Yeah. Looks like SSP_PATTERN, LOG_TYPE and logCheckpoint are unnecessary. Removing them.
On Oct. 27, 2014, 11 p.m., Navina Ramesh wrote:
> > Also, I want to confirm--you're developing this off of the 0.8.0 branch, not master, right?
I am developing off master branch :( Should I develop this off the 0.8.0 branch ?
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58703
-----------------------------------------------------------
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
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Chris Riccomini <cr...@apache.org>.
On Oct. 27, 2014, 11 p.m., Navina Ramesh wrote:
> > Also, I want to confirm--you're developing this off of the 0.8.0 branch, not master, right?
>
> Navina Ramesh wrote:
> I am developing off master branch :( Should I develop this off the 0.8.0 branch ?
>
> Chris Riccomini wrote:
> Yeah, develop off of 0.8. The reason I say that is that 0.9 (master) is currently very turbulent due to my changes, and things like TaskNameToSystemStreamPartition are gone in master.
>
> Since this tool is to migrate from 0.7 to 0.8, I think we should just include it in the 0.8 branch (not master).
Also, regarding testing, did you validate that the offsets were properly restored after the migration? It should be shown in the logs.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27246/#review58703
-----------------------------------------------------------
On Oct. 28, 2014, 2:06 a.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27246/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2014, 2:06 a.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. Upgraded to 0.8 version and restarted the job. Works fine
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 27246: Adding checkpoint migration tool LISAMZA-594
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
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
>
>