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/22 01:24:27 UTC
Review Request 50318: SAMZA-979: Fix for KafkaCheckpointMigration not
registering source correctly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/
-----------------------------------------------------------
Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
Repository: samza
Description
-------
After the thread-safe kafka producer change, all producer need to register the source before publishing to it. The registering of KafkaCheckpointMigration happens in the wrong place. Move it after the producer is created.
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-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
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
>
>
Re: Review Request 50318: SAMZA-979: Remove KafkaCheckpointMigration
Posted by Xinyu Liu <xi...@gmail.com>.
-----------------------------------------------------------
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: Fix for KafkaCheckpointMigration
not registering source correctly
Posted by Navina Ramesh <nr...@linkedin.com>.
> On July 22, 2016, 10:04 p.m., Yi Pan (Data Infrastructure) wrote:
> > Do we still want to keep this??? We do not support migration from 0.9 directly to 0.11. And the code here is to migrate 0.9 jobs to 0.10 that have the changlog partition map from checkpoint to the coordinator stream. I would propose to remove the migration code completely.
Yes. I think we should remove this code ! +1 for that :)
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143283
-----------------------------------------------------------
On July 22, 2016, 1:24 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 22, 2016, 1:24 a.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> After the thread-safe kafka producer change, all producer need to register the source before publishing to it. The registering of KafkaCheckpointMigration happens in the wrong place. Move it after the producer is created.
>
> 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-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
>
> 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: Fix for KafkaCheckpointMigration
not registering source correctly
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50318/#review143283
-----------------------------------------------------------
Do we still want to keep this??? We do not support migration from 0.9 directly to 0.11. And the code here is to migrate 0.9 jobs to 0.10 that have the changlog partition map from checkpoint to the coordinator stream. I would propose to remove the migration code completely.
- Yi Pan (Data Infrastructure)
On July 22, 2016, 1:24 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50318/
> -----------------------------------------------------------
>
> (Updated July 22, 2016, 1:24 a.m.)
>
>
> Review request for samza, Navina Ramesh and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> After the thread-safe kafka producer change, all producer need to register the source before publishing to it. The registering of KafkaCheckpointMigration happens in the wrong place. Move it after the producer is created.
>
> 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-kafka/src/main/scala/org/apache/samza/migration/KafkaCheckpointMigration.scala 5d2641ab1e54d49f7b983bc526762cfb50f2911b
>
> Diff: https://reviews.apache.org/r/50318/diff/
>
>
> Testing
> -------
>
> Tested by locally deployed jobs.
>
> Passed gradle tests.
>
>
> Thanks,
>
> Xinyu Liu
>
>