> On May 13, 2014, 8:48 p.m., Chris Riccomini wrote: > > samza-api/src/main/java/org/apache/samza/task/TaskCoordinator.java, line 64 > > <https://reviews.apache.org/r/21014/diff/2/?file=579748#file579748line64> > > > > nit: "or to all tasks in the container"
Thanks, fixed > On May 13, 2014, 8:48 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 57 > > <https://reviews.apache.org/r/21014/diff/2/?file=579749#file579749line57> > > > > Can we just create one ReadableCoordinator here, and then call > > process(coordinator), window(coordinator), check(coordinator)? It seems > > safe (at first glance), and would save us from having to remember to create > > a coordinator and call check every time we want to use it in the methods > > below. I think that would get quite messy. ReadableCoordinator is now specific to one particular TaskInstance (it takes the Partition as a constructor argument -- will be the task name post SAMZA-71), so that when shutdown/commit for current task is requested, we know which task we're talking about. That doesn't mesh well with passing the same coordinator to process and window: `process` calls through to zero or one TaskInstances (but we only know which one after we've invoked the chooser), whereas `window` calls through to all or none of the TaskInstances. There is no significant performance advantage to reusing the coordinator: `window` only creates ReadableCoordinators when the window time has elapsed, which is usually much less frequent than `process`. So I'd prefer to leave it as it is. > On May 13, 2014, 8:48 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 84 > > <https://reviews.apache.org/r/21014/diff/2/?file=579749#file579749line84> > > > > nit: white space is a little odd here I deliberately added these blank lines, thinking that they improved readability. But I'm happy to remove them if you prefer. > On May 13, 2014, 8:48 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala, line 154 > > <https://reviews.apache.org/r/21014/diff/2/?file=579749#file579749line154> > > > > debut() message here Ok, done. - Martin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21014/#review42877 ----------------------------------------------------------- On May 13, 2014, 2:11 p.m., Martin Kleppmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21014/ > ----------------------------------------------------------- > > (Updated May 13, 2014, 2:11 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > SAMZA-253: Consensus shutdown API > > > Diffs > ----- > > samza-api/src/main/java/org/apache/samza/task/TaskCoordinator.java > 192b226c9e6105cf666d484ac33bb0f854de7688 > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 24364f4ad967eec9474225604b9cc4f830cc3b2e > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala > c4b135c0f46edaa7fc0e4c0bf909e1ffa9515242 > > samza-core/src/main/scala/org/apache/samza/container/TaskInstanceMetrics.scala > 46f1f174b87c40e296cae17dcaf190463b2acdfb > samza-core/src/main/scala/org/apache/samza/task/ReadableCoordinator.scala > aaf631ec7acd710ab8a5b288f696233223569b60 > samza-core/src/test/scala/org/apache/samza/container/TestRunLoop.scala > PRE-CREATION > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > 699f6a9f6bf23fe24784b95569e82a54fcce9553 > samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala > 27b4ca5d7995536aa66f63b7329caddf41865bb4 > > samza-core/src/test/scala/org/apache/samza/task/TestReadableCoordinator.scala > c45ed9bb1b3916cd4c4043e36272b4e3508bfb87 > > samza-test/src/main/java/org/apache/samza/test/integration/SimpleStatefulTask.java > 46f6ef6bbdcac1f8ff7bb97fe709c357d86e0b3f > > samza-test/src/main/java/org/apache/samza/test/integration/StatePerfTestTask.java > d84db5e2fa9feccf2e5257801cde0ec2916e6fc0 > > samza-test/src/main/java/org/apache/samza/test/integration/join/Emitter.java > b3be6531ab537c820ace05011f727b7697c47bd4 > > samza-test/src/main/scala/org/apache/samza/test/performance/TestPerformanceTask.scala > 49dffa142d3ba8acbca07a80c5e4c0d87c6d8551 > > samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala > 6fdfcfced66252369002c5e983c7ef9ed989c171 > > Diff: https://reviews.apache.org/r/21014/diff/ > > > Testing > ------- > > > Thanks, > > Martin Kleppmann > >
