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 52476: SAMZA-1083 : Do not load task store which are older than delete tombstones.
Date Wed, 08 Feb 2017 22:06:45 GMT

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


Fix it, then Ship it!




LGTM, thanks. Few code style/documentation related comments.


samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 107)
<https://reviews.apache.org/r/52476/#comment236555>

    Can delete, doesn't describe the entire block, and unnecessary for the first part.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 113)
<https://reviews.apache.org/r/52476/#comment236557>

    s/of the store/for the store.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 120)
<https://reviews.apache.org/r/52476/#comment236554>

    @param doesn't go in the method description, use {@code}



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 135)
<https://reviews.apache.org/r/52476/#comment236559>

    Explain what stale means here.
    
    Use @code instead of @param here.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 138)
<https://reviews.apache.org/r/52476/#comment236561>

    Can just say "true if the store is stale". Description of what stale means should go in
the method description.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 159)
<https://reviews.apache.org/r/52476/#comment236562>

    No comma before 'if' if it's the last clause in the sentence.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 196)
<https://reviews.apache.org/r/52476/#comment236565>

    s/in the store/for the store.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 198)
<https://reviews.apache.org/r/52476/#comment236563>

    No space before colon, here and other places in this file.



samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala (line 202)
<https://reviews.apache.org/r/52476/#comment236564>

    Thanks!


- Prateek Maheshwari


On Feb. 8, 2017, 1:37 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52476/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2017, 1:37 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Every local task store is backed up by a kafka changelog topic. Due to log compaction,
delete tombstones of the changelog topic have a ttl of delete.retention.ms. Replaying the
events from the changelog that has missing delete tombstones, would result in creation of
an inconsistent local store(due to the missing of some delete events). This patch deletes
the local stores in which difference between current time and last modified time of the offset
file is greater than delete.retention.ms during the container startup.
> 
> 
> Diffs
> -----
> 
>   samza-core/src/main/java/org/apache/samza/storage/StorageRecovery.java 9329edf7d724f3a0d9235354bb77936f713e3b5f

>   samza-core/src/main/scala/org/apache/samza/config/StorageConfig.scala a3587d0a40c57374ee1742234929d444e381e42d

>   samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala c3308bfd7de04c335fef6cb66baa29286a230080

>   samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala 0b7bcdda1639eea8239a69c31bdf42558e9077d2

>   samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala 4d40f520e54beb643acd8410c772b75e2f6a9162

>   samza-kafka/src/main/scala/org/apache/samza/config/KafkaConfig.scala 9320cf744ff90d647a198b51cb06d2a526fe68fa

> 
> Diff: https://reviews.apache.org/r/52476/diff/
> 
> 
> Testing
> -------
> 
> Unit testing and manual testing has been done to verify the functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>


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