samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>
Subject Re: Review Request 36903: SAMZA-744: shutdown stores before shutdown producers
Date Thu, 06 Aug 2015 11:26:51 GMT


> 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)
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message