You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com> on 2015/07/29 23:48:31 UTC
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/
-----------------------------------------------------------
(Updated July 29, 2015, 9:48 p.m.)
Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
Summary (updated)
-----------------
SAMZA-744: shutdown stores before shutdown producers
Bugs: SAMZA-744
https://issues.apache.org/jira/browse/SAMZA-744
Repository: samza
Description
-------
SAMZA-744: shutdown stores before shutdown producers
Diffs
-----
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
Diff: https://reviews.apache.org/r/36903/diff/
Testing (updated)
-------
./bin/check-all.sh passed
Thanks,
Yi Pan (Data Infrastructure)
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/#review93524
-----------------------------------------------------------
Ship it!
Ship It!
- Navina Ramesh
On July 29, 2015, 9:48 p.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36903/
> -----------------------------------------------------------
>
> (Updated July 29, 2015, 9:48 p.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
>
>
> Bugs: SAMZA-744
> https://issues.apache.org/jira/browse/SAMZA-744
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-744: shutdown stores before shutdown producers
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
>
> Diff: https://reviews.apache.org/r/36903/diff/
>
>
> Testing
> -------
>
> ./bin/check-all.sh passed
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/#review94572
-----------------------------------------------------------
Ship it!
lgtm!
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownStatefulTask.scala (line 131)
<https://reviews.apache.org/r/36903/#comment149178>
nit: remove ';'
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownStatefulTask.scala (line 135)
<https://reviews.apache.org/r/36903/#comment149179>
nit: same. remove ';'
- Navina Ramesh
On Aug. 6, 2015, 11:24 a.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36903/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2015, 11:24 a.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
>
>
> Bugs: SAMZA-744
> https://issues.apache.org/jira/browse/SAMZA-744
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-744: shutdown stores before shutdown producers
>
>
> Diffs
> -----
>
> build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
> samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTestUtil.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownStatefulTask.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala ea702a919348305ff95ce0b4ca1996a13aff04ec
>
> Diff: https://reviews.apache.org/r/36903/diff/
>
>
> Testing
> -------
>
> ./bin/check-all.sh passed
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/
-----------------------------------------------------------
(Updated Aug. 6, 2015, 11:24 a.m.)
Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
Changes
-------
Addressed the comments on test code refactoring.
Bugs: SAMZA-744
https://issues.apache.org/jira/browse/SAMZA-744
Repository: samza
Description
-------
SAMZA-744: shutdown stores before shutdown producers
Diffs (updated)
-----
build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTestUtil.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownStatefulTask.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala ea702a919348305ff95ce0b4ca1996a13aff04ec
Diff: https://reviews.apache.org/r/36903/diff/
Testing
-------
./bin/check-all.sh passed
Thanks,
Yi Pan (Data Infrastructure)
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala, lines 47-49
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029022#file1029022line47>
> >
> > we need to remove the author information. :) And maybe add some java doc instead.
> >
> > My 2 cents:
> > 1. If this is a real test, to be consistent, we may want to use TestStreamTask (begin with Test), or change all other TestSomething to SomethingTest (e.g. change TestStateful to StatefulTest)
> >
> > 2. If this is not a real test, I prefer something like StreamTaskUtil to be less ambiguous.
Done.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala, line 96
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029022#file1029022line96>
> >
> > is this tag used?
Removed.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala, line 148
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029022#file1029022line148>
> >
> > same, is this used?
Removed
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala, line 169
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029022#file1029022line169>
> >
> > There is no "TestJob". (I know, it is copy/paste issue :)
Fixed.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala, line 64
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029023#file1029023line64>
> >
> > From the description, it is not testing the Container Shutdown, actually it is testing the store restoring feature.
Updated the description
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala, line 66
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029023#file1029023line66>
> >
> > Since we already are doing the abstraction, is it possible to put the common config into StreamTastTest object? Becaue I see a lot of the same configs in ShutdownContainerTest and TestStatefulTask.
Done.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala, lines 87-89
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029023#file1029023line87>
> >
> > in the 0.10.0, we do not have checkpoint factory, I believe
There is issues w/ removing. Opened JIRA SAMZA-754 for it.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala, lines 142-146
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029023#file1029023line142>
> >
> > are those two methods used anywhere?
Removed.
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala, line 165
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029023#file1029023line165>
> >
> > how about adding override?
Done
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala, line 227
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029024#file1029024line227>
> >
> > actually i do not understand why we need a companion object here. We just use the default task number, 1.
> >
> > And awaitTaskRegistered and register methods are not used anywhere.
Removed
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala, lines 32-34
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029025#file1029025line32>
> >
> > Instead of the author information, I think putting some java doc explaining this class/object will be better.
Fixed
> On Aug. 5, 2015, 7:48 a.m., Yan Fang wrote:
> > samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala, line 37
> > <https://reviews.apache.org/r/36903/diff/2/?file=1029025#file1029025line37>
> >
> > rm ";"
Fixed
- Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/#review94193
-----------------------------------------------------------
On Aug. 6, 2015, 11:24 a.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36903/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2015, 11:24 a.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
>
>
> Bugs: SAMZA-744
> https://issues.apache.org/jira/browse/SAMZA-744
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-744: shutdown stores before shutdown producers
>
>
> Diffs
> -----
>
> build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
> samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTestUtil.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownStatefulTask.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala ea702a919348305ff95ce0b4ca1996a13aff04ec
>
> Diff: https://reviews.apache.org/r/36903/diff/
>
>
> Testing
> -------
>
> ./bin/check-all.sh passed
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/#review94193
-----------------------------------------------------------
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala (lines 47 - 49)
<https://reviews.apache.org/r/36903/#comment148730>
we need to remove the author information. :) And maybe add some java doc instead.
My 2 cents:
1. If this is a real test, to be consistent, we may want to use TestStreamTask (begin with Test), or change all other TestSomething to SomethingTest (e.g. change TestStateful to StatefulTest)
2. If this is not a real test, I prefer something like StreamTaskUtil to be less ambiguous.
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala (line 96)
<https://reviews.apache.org/r/36903/#comment148740>
is this tag used?
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala (line 148)
<https://reviews.apache.org/r/36903/#comment148741>
same, is this used?
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala (line 169)
<https://reviews.apache.org/r/36903/#comment148742>
There is no "TestJob". (I know, it is copy/paste issue :)
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala (line 176)
<https://reviews.apache.org/r/36903/#comment148752>
why TestStateStoreTask here? I think you mean TestTask.awaitTaskReistered
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (line 64)
<https://reviews.apache.org/r/36903/#comment148745>
From the description, it is not testing the Container Shutdown, actually it is testing the store restoring feature.
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (line 66)
<https://reviews.apache.org/r/36903/#comment148734>
Since we already are doing the abstraction, is it possible to put the common config into StreamTastTest object? Becaue I see a lot of the same configs in ShutdownContainerTest and TestStatefulTask.
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (lines 87 - 89)
<https://reviews.apache.org/r/36903/#comment148755>
in the 0.10.0, we do not have checkpoint factory, I believe
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (lines 142 - 146)
<https://reviews.apache.org/r/36903/#comment148754>
are those two methods used anywhere?
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (line 155)
<https://reviews.apache.org/r/36903/#comment148758>
how about adding override ?
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala (line 165)
<https://reviews.apache.org/r/36903/#comment148759>
how about adding override?
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala (lines 88 - 91)
<https://reviews.apache.org/r/36903/#comment148756>
same
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala (line 95)
<https://reviews.apache.org/r/36903/#comment148757>
actually i do not understand why we need a companion object here. We just use the default task number, 1.
And awaitTaskRegistered and register methods are not used anywhere.
samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala (lines 32 - 34)
<https://reviews.apache.org/r/36903/#comment148731>
Instead of the author information, I think putting some java doc explaining this class/object will be better.
samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala (line 37)
<https://reviews.apache.org/r/36903/#comment148749>
rm ";"
- Yan Fang
On Aug. 4, 2015, 9:30 p.m., Yi Pan (Data Infrastructure) wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36903/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 9:30 p.m.)
>
>
> Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
>
>
> Bugs: SAMZA-744
> https://issues.apache.org/jira/browse/SAMZA-744
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-744: shutdown stores before shutdown producers
>
>
> Diffs
> -----
>
> build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
> samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala ea702a919348305ff95ce0b4ca1996a13aff04ec
> samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36903/diff/
>
>
> Testing
> -------
>
> ./bin/check-all.sh passed
>
>
> Thanks,
>
> Yi Pan (Data Infrastructure)
>
>
Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown
producers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36903/
-----------------------------------------------------------
(Updated Aug. 4, 2015, 9:30 p.m.)
Review request for samza, Yan Fang, Chinmay Soman, Chris Riccomini, and Navina Ramesh.
Changes
-------
Added integration tests and verified the fix.
- Restructured the stateful task tests s.t. it is easier to add more tests.
Bugs: SAMZA-744
https://issues.apache.org/jira/browse/SAMZA-744
Repository: samza
Description
-------
SAMZA-744: shutdown stores before shutdown producers
Diffs (updated)
-----
build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 27b2517048ad5730762506426ee7578c66181db8
samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTest.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/test/integration/TestShutdownContainer.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala ea702a919348305ff95ce0b4ca1996a13aff04ec
samza-test/src/test/scala/org/apache/samza/test/integration/TestTask.scala PRE-CREATION
Diff: https://reviews.apache.org/r/36903/diff/
Testing
-------
./bin/check-all.sh passed
Thanks,
Yi Pan (Data Infrastructure)