> On April 14, 2015, 9:16 p.m., Chris Riccomini wrote:
> > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala,
> > line 26
> > <https://reviews.apache.org/r/33146/diff/1/?file=926491#file926491line26>
> >
> > Prefer not to introduce a dependency on google common.
Removed and replaced with Scala operations to reduce debt in case we want to
remove said dependency, which seems to be required for
UnsignedBytes.lexicographicalComparator in InMemoryKeyValueStore (and not just
tests):
project(":samza-kv-inmemory_$scalaVersion") {
...
compile "com.google.guava:guava:$guavaVersion"
testCompile "junit:junit:$junitVersion"
}
}
- Mohamed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33146/#review80110
-----------------------------------------------------------
On April 16, 2015, 10:43 a.m., Mohamed Mahmoud (El-Geish) wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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.
> * 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
> -----
>
>
> 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)
>
>
