----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48182/#review136075 -----------------------------------------------------------
Fix it, then Ship it! samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala (line 67) <https://reviews.apache.org/r/48182/#comment201048> It looks like this can be broken out into a second test. The first seems to be testing a simple flush where as this one appears to be testing the behavior of remove with iterators. samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala (line 40) <https://reviews.apache.org/r/48182/#comment201049> very thread safe? :) samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala (line 195) <https://reviews.apache.org/r/48182/#comment201050> At the very least I would add a comment that the lock must be held to call this method. samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (lines 409 - 410) <https://reviews.apache.org/r/48182/#comment201052> I'm not totally sure what you're trying to do with this test, but if you want to guarantee that runner1 only starts after runner2 then you should use a count down latch. samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (lines 412 - 413) <https://reviews.apache.org/r/48182/#comment201051> It's generally a good idea to join with a timeout. If there were a bug this would hang the test rather than cause a test failure. samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (lines 505 - 506) <https://reviews.apache.org/r/48182/#comment201066> Asserts on another thread are not going to work correctly. They won't fail the test - they will kill the thread. samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (line 519) <https://reviews.apache.org/r/48182/#comment201064> Isn't there a race here if this code is invoked before the store.put on line 492? - Chris Pettitt On June 2, 2016, 6:33 p.m., Xinyu Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48182/ > ----------------------------------------------------------- > > (Updated June 2, 2016, 6:33 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 > >
