samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mohamed Mahmoud (El-Geish)" <elge...@gmail.com>
Subject Re: Review Request 33146: New KeyValueStore Features
Date Sun, 26 Apr 2015 07:38:11 GMT


> On April 24, 2015, 5:01 p.m., Yi Pan (Data Infrastructure) wrote:
> > Ship It!

I don't have access to commit. Can you please grant me access or commit for me? Thanks!


- Mohamed


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


On April 24, 2015, 4:59 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 24, 2015, 4:59 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Bugs: SAMZA-647
>     https://issues.apache.org/jira/browse/SAMZA-647
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> * Adding new KeyValueStore methods: Map<K, V> getAll(List<K>), and void deleteAll(List<K>).
> **Please note: "Backwards incompatible API changes, config changes, or library upgrades
should only happen between major revision changes, or when the major revision is 0." -- since
the latter is true, I found that adding the new methods to the already-existing interface
to be a better solution, even though it breaks backward compatability (please see the first
iteration for context).** Alternatively, to maintain backward compat., I would have added
a new contract and a new class for each class that implements KeyValueStore (to implement
the new contract and pass calls throught to the underlying store, whose type needs to change
in the constructor to the new contract).
> * Improved the javadoc of KeyValueStore to have the same voice, to add API notes, and
to follow the javadoc standards.
> * Removing stress tests from TestKeyValueStores.scala because:
> * * Unit tests are not meant to be stress-testing the system,
> * * The test load looked arbitrary,
> * * It wasn't measuring anything (just testing it doesn't crash),
> * * Stress testing requires extended periods of testing, and
> * * My machine, a MacBook Air, is not beefy enough to survive stress tests; they should
be run on a typical production machine and not a dev one -- a flaky test is not a good test.
> * * There's a test class dedicated for KV stores' performance testing: TestKeyValuePerformance
> * Last, but definitely not least, the main motiviation behind this change: Allowing RocksDbKeyValueStore
to implement getAll(List<K>) 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).
> 
> 
> 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/KeyValueStore.java b708341abed15aaad34df5934f5f310bc1feb87a

>   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/main/scala/org/apache/samza/storage/kv/KeyValueStoreMetrics.scala 79092b91c9498e55f1c4e28661b7280c6c19cef7

>   samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStore.scala 26f4cd9cfef305546c85ef9330f3e8b8be5336f7

>   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/main/config/perf/kv-perf.properties 33fcd8d1aea14ecea47bbadb24936f737feedb39

>   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