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