----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33146/#review80036 -----------------------------------------------------------
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. samza-kv/src/main/java/org/apache/samza/storage/kv/BatchingKeyValueStore.java <https://reviews.apache.org/r/33146/#comment129786> Nit: indentation should be 2 spaces. samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStoreMetrics.scala <https://reviews.apache.org/r/33146/#comment129787> Do we need a deleteAlls metric as well? - Chris Riccomini 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) > >
