> On Nov. 8, 2016, 11:52 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, > > line 239 > > <https://reviews.apache.org/r/53453/diff/2/?file=1554966#file1554966line239> > > > > Are you missing a foreach here? > > > > I think you need something like: > > lastProcessedOffsets.get(taskName) > > .foreach { sspToOffsets => sspToOffsets.foreach { case (ssp, > > checkpoint) => > > offsetManagerMetrics.checkpointedOffsets(ssp).set(checkpoint) } } > > > > If the version above is correct, can we add a test for this? Its easy > > to miss.
Interesting, Scala was doing the right thing, although it is not clear why. I had some tests for it and they passed. So I rewrote this part to make it clearer. > On Nov. 8, 2016, 11:52 p.m., Prateek Maheshwari wrote: > > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, > > line 77 > > <https://reviews.apache.org/r/53453/diff/2/?file=1554966#file1554966line77> > > > > Strongly prefer not adding empty Map() as default value here (and maybe > > clean up the other one too). See my comment on Xinyu's HDFS performance RB > > for explanation. In general I agree with you, but in this case it is kind of hard to do it, because having a parameter without a default between parameters with default will confuse compiler when only some of the arguments are provided. - Boris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53453/#review155377 ----------------------------------------------------------- On Nov. 4, 2016, 11:23 p.m., Boris Shkolnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53453/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2016, 11:23 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-1042 > https://issues.apache.org/jira/browse/SAMZA-1042 > > > Repository: samza > > > Description > ------- > > Add optional interface for SystemConsumer checkpontListener() for checkpoint > notifications. > > > Diffs > ----- > > samza-api/src/main/java/org/apache/samza/checkpoint/CheckpointListener.java > PRE-CREATION > samza-api/src/main/java/org/apache/samza/system/SystemConsumer.java > 8dfcc7499659442aabd3085a8787475fe38f29db > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala > c41eadb70f4675816245f7ac40f0db2fc16335f0 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > e0468ee89c89fd720834461771ebb36475475bcb > > samza-core/src/test/scala/org/apache/samza/checkpoint/TestOffsetManager.scala > cb78223f1b59a78bbeb1e42b5974670a53def504 > > Diff: https://reviews.apache.org/r/53453/diff/ > > > Testing > ------- > > gradlew test. > manual testing. > > > Thanks, > > Boris Shkolnik > >
