samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Prateek Maheshwari <pmaheshw...@linkedin.com>
Subject Re: Review Request 53453: Add optional interface for SystemConsumer checkpontListener() for checkpoint notifications
Date Tue, 08 Nov 2016 23:52:55 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53453/#review155377
-----------------------------------------------------------



Some minor code style/documentation related comments. One correctness question.


samza-api/src/main/java/org/apache/samza/checkpoint/CheckpointListener.java (line 25)
<https://reviews.apache.org/r/53453/#comment225248>

    No comma after SystemConsumers



samza-api/src/main/java/org/apache/samza/system/SystemConsumer.java (line 134)
<https://reviews.apache.org/r/53453/#comment225249>

    s/checkpoint/register the same offset.
    
    Also, maintain previous line width for comment.



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 77)
<https://reviews.apache.org/r/53453/#comment225250>

    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.



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 146)
<https://reviews.apache.org/r/53453/#comment225252>

    s/ones/one



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 239)
<https://reviews.apache.org/r/53453/#comment225255>

    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.



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 243)
<https://reviews.apache.org/r/53453/#comment225257>

    Can remove comment, same information in next line.



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 244)
<https://reviews.apache.org/r/53453/#comment225258>

    Space after 'case' and after ':'



samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 245)
<https://reviews.apache.org/r/53453/#comment225259>

    s/is an empty list/is empty



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 361)
<https://reviews.apache.org/r/53453/#comment225260>

    Indent by 2
    Space before and after =>
    Space b/w map and {



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 366)
<https://reviews.apache.org/r/53453/#comment225261>

    Indent by 2.


- Prateek Maheshwari


On Nov. 4, 2016, 4: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, 4: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