----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48182/#review135984 -----------------------------------------------------------
Fix it, then Ship it! some nits.. otherwise, +1 ! samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/48182/#comment200993> Wow.. wonder why this variable was created :/ samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala (line 90) <https://reviews.apache.org/r/48182/#comment201377> I don't understand the point of this test. If you testing removal, what is the need to iterate? If you are testing iterator behavior, then, perhaps the method should be renamed?? Either ways, some javadocs about this test can help! :) samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala (line 109) <https://reviews.apache.org/r/48182/#comment201376> nit: unused variable? - Navina Ramesh On June 3, 2016, 9:30 p.m., Xinyu Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48182/ > ----------------------------------------------------------- > > (Updated June 3, 2016, 9:30 p.m.) > > > Review request for samza, Chris Pettitt, Navina Ramesh, and Yi Pan (Data > Infrastructure). > > > Repository: samza > > > Description > ------- > > All the existing stores/cache need to be thread safe in order to be used by > multithreaded tasks. The following changes are made to ensure the thread > safety of the stores: > > For CachedStore, use sychronized lock for each public function; > For current InMemoryKeyValueStore, use ConcurrentSkipListMap as underlying > map for thread safety. > For store Iterator, do not support remove functionality (throw > UnsupportedOperationException like RocksDb does today). > > > Diffs > ----- > > > samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala > 72f25a354eaa98e8df379d07d9cc8613dfafd13a > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala > f0965aec5f3ec2a214dc40c70832c58273623749 > > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala > b7f1cdc4dbaeea2413cee2ad60d74528f3950513 > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala > c28f8db8cb59bd5415e78535877acc1e5bee0f67 > samza-kv/src/test/scala/org/apache/samza/storage/kv/TestCachedStore.scala > eee74473726cb2a36d0b75fe5c9df737440980bc > > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala > 23f8a1a6bee8ef38e0640a4e90778e53d982deeb > > Diff: https://reviews.apache.org/r/48182/diff/ > > > Testing > ------- > > Unit tests and local deployment. > > > Thanks, > > Xinyu Liu > >
