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 Thu, 16 Apr 2015 10:43:43 GMT

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

(Updated April 16, 2015, 10:43 a.m.)


Review request for samza.


Changes
-------

Please see the new description for details about this iteration.


Summary (updated)
-----------------

New KeyValueStore Features


Bugs: SAMZA-647
    https://issues.apache.org/jira/browse/SAMZA-647


Repository: samza


Description (updated)
-------

* 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.
* Making the KeyValueStore extend AutoClosable and Flushable (since we can use Java 1.7 now).
* 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 (updated)
-----

  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