> On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, > > line 106 > > <https://reviews.apache.org/r/33146/diff/2/?file=931565#file931565line106> > > > > From the put() method below, null values seem to be impossible: > > > > if (value == null) { > > db.remove(writeOptions, key) > > ...
You're assuming that RocksDB never returns a null value (what we put is irrelevant beceause RocksDB can change the implementation behind our backs); I'd rather be cynical and guard against NPE here. > On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java, line > > 33 > > <https://reviews.apache.org/r/33146/diff/2/?file=931566#file931566line33> > > > > The signature of close() and flush() functions from AutoCloseable and > > Flushable are different from what the existing function signatures. > > Wouldn't that cause backward-compatibility issues w/ the user code? I would > > prefer to do the minimum to just add the new methods, instead of changing > > the existing ones. > > Yi Pan (Data Infrastructure) wrote: > Actually, reading @Chris comments on SAMZA-425, I start thinking that it > might be the time for us to add JDK7/8 only features in the code. But that > would need a wider range discussion and agreement. I would prefer to put this > to discussion or to a separate JIRA first, if you don't mind. A- I'm not as concerned about breaking client code right now per the note in the description of this change B- I'm sure we are only allowed Java 7 features right now, 8 is another discussion to be had C- "I would prefer to put this to discussion or to a separate JIRA first", agreed, but I'm not sure what change you're referring to? Using Java 7 features (this has been discussed in SAMZA-455, which is resolved)? Or making this interface extend AutoClosable and Flushable? (Sounds isomorphic to SAMZA-425, which is approved) -- I removed the latter to unblock this RB. > On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java, line > > 41 > > <https://reviews.apache.org/r/33146/diff/2/?file=931566#file931566line41> > > > > The methods seemed to be re-ordered and it makes it a bit difficult to > > identify the exact changes. Could you turn off the re-ordering in the > > format template s.t. we keep the change minimum? I didn't re-order the methods; there are extensive docs changes which RB fail to diff cleanly; it's a very small file; I suggest reviewing the new version as if it's a newly added file. If you feel strongly about it, I can separate the changes in two RBs. > On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala, line > > 105 > > <https://reviews.apache.org/r/33146/diff/2/?file=931567#file931567line105> > > > > This could be more concise with keys.filter(...) and misses.foreach(...) A- I'd rather be consistent with the current file style, please see putAll() B- I'm new to Scala, and I was worried about any hidden cost of converting the java iterator to a scala one, but seems like it's happening implicitly regardless. > On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStore.scala, line > > 96 > > <https://reviews.apache.org/r/33146/diff/2/?file=931570#file931570line96> > > > > nit: can be concise as keys.foreach(key => collector.send(new > > OutgoingMessageEnvelope(systemStream, partitionId, key, null)) Ditto. > On April 21, 2015, 6:49 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala, > > line 133 > > <https://reviews.apache.org/r/33146/diff/2/?file=931574#file931574line133> > > > > nit: prefer not to re-order the methods if not necessary. Again, no re-ordering happened here either, just RB doing a poor job at diffing the changes. - Mohamed ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33146/#review81004 ----------------------------------------------------------- On April 23, 2015, 7:44 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 23, 2015, 7:44 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) > >
