You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@samza.apache.org by Guozhang Wang <wa...@gmail.com> on 2015/05/01 20:43:18 UTC

Review Request 33761: Fix SAMZA-658

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

Review request for samza.


Repository: samza


Description
-------

Add removing logic to the intermediate cached store for kv engine


Diffs
-----

  samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala b3624e6057ee1a86090f00d2853035b06f63358d 
  samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala 61bb3f6acb080b653f8b11176538549738255acc 
  samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala 3a23daf053f0b8dec3a7ec83a51c9c5527078a3b 

Diff: https://reviews.apache.org/r/33761/diff/


Testing
-------


Thanks,

Guozhang Wang

Re: Review Request 33761: Fix SAMZA-658

Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.

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


Overall looks good. Just some minor comments.


samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala
<https://reviews.apache.org/r/33761/#comment133359>

    Refer to in-memory KV-store class, we can merge those two in one function: getIter(iter) and make range() and all() referring to the same function like:
    
    def range(from: K, to: K) = {
      metrics.ranges.inc
      flush()
      getIter(store.range(from, to))
    }



samza-kv/src/test/scala/org/apache/samza/storage/kv/TestCachedStore.scala
<https://reviews.apache.org/r/33761/#comment133362>

    It would be better to test iter.remove() for both range() and all()


- Yi Pan (Data Infrastructure)


On May 4, 2015, 9:07 p.m., Guozhang Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33761/
> -----------------------------------------------------------
> 
> (Updated May 4, 2015, 9:07 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Bugs: SAMZA-658
>     https://issues.apache.org/jira/browse/SAMZA-658
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Address Yi's comments, add unit test
> 
> 
> Diffs
> -----
> 
>   samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala b3624e6057ee1a86090f00d2853035b06f63358d 
>   samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala 61bb3f6acb080b653f8b11176538549738255acc 
>   samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala 3a23daf053f0b8dec3a7ec83a51c9c5527078a3b 
>   samza-kv/src/test/scala/org/apache/samza/storage/kv/MockKeyValueStore.scala PRE-CREATION 
>   samza-kv/src/test/scala/org/apache/samza/storage/kv/TestCachedStore.scala d03ec925b103ccf3c1561de0461fbc39cbe9d9ca 
> 
> Diff: https://reviews.apache.org/r/33761/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Guozhang Wang
> 
>