You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Xinyu Liu <xi...@gmail.com> on 2016/07/25 21:29:36 UTC
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/
-----------------------------------------------------------
(Updated July 25, 2016, 9:29 p.m.)
Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
Changes
-------
Remove the kafka checkpoint migration code according to the feedback.
Summary (updated)
-----------------
SAMZA-979: Remove KafkaCheckpointMigration
Repository: samza
Description (updated)
-------
KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
Also find an extra logging introduced by the change. Remove it during this fix.
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
Diff: https://reviews.apache.org/r/50318/diff/
Testing
-------
Tested by locally deployed jobs.
Passed gradle tests.
Thanks,
Xinyu Liu
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by Navina Ramesh <nr...@linkedin.com>.
> On July 25, 2016, 11:19 p.m., Navina Ramesh wrote:
> > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, line 226
> > <https://reviews.apache.org/r/50318/diff/2/?file=1452069#file1452069line226>
> >
> > This info looks pretty useful. Any particular reason you are removing this ?
>
> Xinyu Liu wrote:
> oh, I added this line of logging by accident in the patch for multithreading. It might cause too much logging.
Ok !
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143446
-----------------------------------------------------------
On July 25, 2016, 9:29 p.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 25, 2016, 9:29 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
>
> Also find an extra logging introduced by the change. Remove it during this fix.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
> samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
> samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
> samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
> samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by Xinyu Liu <xi...@gmail.com>.
> On July 25, 2016, 11:19 p.m., Navina Ramesh wrote:
> > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, line 226
> > <https://reviews.apache.org/r/50318/diff/2/?file=1452069#file1452069line226>
> >
> > This info looks pretty useful. Any particular reason you are removing this ?
oh, I added this line of logging by accident in the patch for multithreading. It might cause too much logging.
- Xinyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143446
-----------------------------------------------------------
On July 25, 2016, 9:29 p.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 25, 2016, 9:29 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
>
> Also find an extra logging introduced by the change. Remove it during this fix.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
> samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
> samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
> samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
> samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143446
-----------------------------------------------------------
Fix it, then Ship it!
One question. Otherwise, looks good! +1
samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala
<https://reviews.apache.org/r/50318/#comment209242>
This info looks pretty useful. Any particular reason you are removing this ?
- Navina Ramesh
On July 25, 2016, 9:29 p.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 25, 2016, 9:29 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
>
> Also find an extra logging introduced by the change. Remove it during this fix.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
> samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
> samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
> samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
> samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
> On July 26, 2016, 10:39 p.m., Boris Shkolnik wrote:
> > Are we sure that noone will try to upgraded from a version before migration to version 11 directly?
We never test upgrading a version from 0.9 to 0.11 for all the changes. Even we keep this code around, the confidence of upgrading directly to 0.11 is pretty low. Hence, I would rather prefer to remove this code.
- Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143632
-----------------------------------------------------------
On July 25, 2016, 9:29 p.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 25, 2016, 9:29 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
>
> Also find an extra logging introduced by the change. Remove it during this fix.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
> samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
> samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
> samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
> samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by Boris Shkolnik <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143632
-----------------------------------------------------------
Ship it!
Are we sure that noone will try to upgraded from a version before migration to version 11 directly?
- Boris Shkolnik
On July 25, 2016, 9:29 p.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 25, 2016, 9:29 p.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> KafkaCheckpointMigration is not needed anymore for ver 11. remove the code.
>
> Also find an extra logging introduced by the change. Remove it during this fix.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala 7245902c69c751a4e8853745de46adf5553d45f5
> samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala a3613ff601131ec8643e407dd89a5b496aa686ea
> samza-core/src/main/scala/org/apache/samza/migration/JobRunnerMigration.scala f38b87ac4a5d3f62e419e0c866e15b5a7ddad26d
> samza-core/src/test/scala/org/apache/samza/job/TestJobRunner.scala e97656aeac270bddcd16248b37312c146d0a7d1b
> samza-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
> samza-kafka/src/test/scala/org/apache/samza/migration/TestKafkaCheckpointMigration.scala 504fc89133702952dcaacb01d06449134b6d8921
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>