samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>
Subject Re: Review Request 33146: Adding a new KV store contract: BatchingKeyValueStore
Date Wed, 15 Apr 2015 18:47:32 GMT


> On April 14, 2015, 9:14 p.m., Chris Riccomini wrote:
> > I'm concerned that there might be an issue with this approach. In BaseKeyValueStorageEngineFactory,
we compose stores by nesting them. If this is the case, I think that the top-most store will
implement the batching key value store, and will use the default implementation of getAll/deleteAll.
As such, I'm not sure that the RocksDB implementation ever actually gets called.

I agree. In addition, it seems that each implementation class of KV store has its own specific
logic. For example, CachedStore.getAll() could mean that only batch get from the db for the
keys *not* currently in cache, while NullSafeKeyValueStore needs to validate each key is not
null before it further calls store.getAll() to trigger the nested KV-store's getAll() method.
It seems that we should just keep the same interface class KeyValueStore<K,V> w/ additional
methods: getAll() and deleteAll(). And each KV-store implementation needs to call the nested
store's getAll() and deleteAll() w/ its own logic.


- Yi


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


On April 13, 2015, 9:24 p.m., Mohamed Mahmoud (El-Geish) wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33146/
> -----------------------------------------------------------
> 
> (Updated April 13, 2015, 9:24 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Bugs: SAMZA-647
>     https://issues.apache.org/jira/browse/SAMZA-647
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> Adding a new KV store contract, BatchingKeyValueStore, which adds the following methods:
> * Map<K, V> getAll(List<K>), and
> * void deleteAll(List<K>)
> 
> Since Samza does not require Java 8, the above cannot be implemented as default interface
method in KeyValueStore, and a new contract (that extends KeyValueStore) is necessary to maintain
backward compatibility.
> Existing KV stores extend the new contract now to be consistent and to enable API callers
to use KeyValueStore or BatchingKeyValueStore interchangeably.
> RocksDbKeyValueStore overrides the getAll behavior to call multiGet(List<K>); Preliminary
tests showed that multiget is at least 1.25x faster per key than get (see https://reviews.facebook.net/rROCKSDB4985a9f73b9fb8a0323fbbb06222ae1f758a6b1d).
> Java source compatibility: 1.6
> 
> 
> Diffs
> -----
> 
>   samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala
217333c84c696c0cc1bc3eeabf1c4066a6e89795 
>   samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
66c2a0dc2e38e21f951727a30f0987776ac52fe2 
>   samza-kv/src/main/java/org/apache/samza/storage/kv/BatchingKeyValueStore.java PRE-CREATION

>   samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala 61bb3f6acb080b653f8b11176538549738255acc

>   samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStoreMetrics.scala 79092b91c9498e55f1c4e28661b7280c6c19cef7

>   samza-kv/src/main/scala/org/apache/samza/storage/kv/NullSafeKeyValueStore.scala 4f48cf490d6c1012591a602c0d29dcc71473090f

>   samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala 531e8bef2069a77fa9ceab36fa738bbaa162fe8c

>   samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea

> 
> Diff: https://reviews.apache.org/r/33146/diff/
> 
> 
> Testing
> -------
> 
> Unit-tested.
> 
> 
> Thanks,
> 
> Mohamed Mahmoud (El-Geish)
> 
>


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