samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boris Shkolnik <bor...@apache.org>
Subject Re: Review Request 53453: Add optional interface for SystemConsumer checkpontListener() for checkpoint notifications
Date Thu, 10 Nov 2016 18:53:13 GMT


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


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