> On March 10, 2015, 3:45 p.m., Chris Riccomini wrote: > > Seems like it should work. Could you write a test to verify? I know it's a > > little hard with shutdown hooks (or anything that interacts with > > System/Runtime Java stuff), but I can't actually prove to myself that this > > works without a test.
Added a unit test that tests as much behavior as I could cover without actually triggering a shutdown, at the cost of making the code in RunLoop messier. > On March 10, 2015, 3:45 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 64 > > <https://reviews.apache.org/r/31881/diff/1/?file=890037#file890037line64> > > > > Nit: move outside of run. Optionally, make shutdownHook injectible via > > constructor for testing purposes. Fixed. > On March 10, 2015, 3:45 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 67 > > <https://reviews.apache.org/r/31881/diff/1/?file=890037#file890037line67> > > > > I think shutdownHook needs to be made volatile. The hook will be > > executed from another thread than the RunLoop, which is reading the > > variable. Good catch, fixed. - Ewen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31881/#review75881 ----------------------------------------------------------- On March 10, 2015, 6:01 p.m., Ewen Cheslack-Postava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31881/ > ----------------------------------------------------------- > > (Updated March 10, 2015, 6:01 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-506 > https://issues.apache.org/jira/browse/SAMZA-506 > > > Repository: samza > > > Description > ------- > > Shutdown RunLoop on SIGTERM. > > > Diffs > ----- > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala 499f5c6 > samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala > ea48853 > > Diff: https://reviews.apache.org/r/31881/diff/ > > > Testing > ------- > > Passes unit and zopkio tests. > > > Thanks, > > Ewen Cheslack-Postava > >
