You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Xinyu Liu <xi...@linkedin.com> on 2016/06/02 18:33:50 UTC
Review Request 48182: SAMZA-958: Make store/cache thread safe
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/
-----------------------------------------------------------
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
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Xinyu Liu <xi...@gmail.com>.
> On June 6, 2016, 6:22 p.m., Chris Pettitt wrote:
> > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala, line 528
> > <https://reviews.apache.org/r/48182/diff/1-2/?file=1405222#file1405222line528>
> >
> > How about actually capturing the test failure and rethrowing from the main thread? It will give you a much more helpful error message.
Good suggestion. Add the try catch and rethrow.
- Xinyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136335
-----------------------------------------------------------
On June 9, 2016, 12:33 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48182/
> -----------------------------------------------------------
>
> (Updated June 9, 2016, 12:33 a.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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Chris Pettitt <cp...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136335
-----------------------------------------------------------
Fix it, then Ship it!
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (line 412)
<https://reviews.apache.org/r/48182/#comment201356>
This might be sufficient, but I've seen some excessively long GC pauses and the like on Hudson. Something like 10s has worked well for me in the past and the join should take nowhere near that long in most cases.
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (lines 490 - 491)
<https://reviews.apache.org/r/48182/#comment201353>
I would name these a little differently to improve readability. Something like runner3StartedLatch and runner1FinishedLatch; or startRunner2 and startRunner1 latch; or similar.
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (line 527)
<https://reviews.apache.org/r/48182/#comment201355>
How about actually capturing the test failure and rethrowing from the main thread? It will give you a much more helpful error message.
- Chris Pettitt
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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Xinyu Liu <xi...@gmail.com>.
> On June 6, 2016, 8:36 p.m., Navina Ramesh wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 90
> > <https://reviews.apache.org/r/48182/diff/2/?file=1406146#file1406146line90>
> >
> > 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! :)
Change the name of the test to match its purpose.
> On June 6, 2016, 8:36 p.m., Navina Ramesh wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 109
> > <https://reviews.apache.org/r/48182/diff/2/?file=1406146#file1406146line109>
> >
> > nit: unused variable?
Good catch! Removed it.
- Xinyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review135984
-----------------------------------------------------------
On June 9, 2016, 12:33 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48182/
> -----------------------------------------------------------
>
> (Updated June 9, 2016, 12:33 a.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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Chris Pettitt <cp...@linkedin.com>.
> On June 9, 2016, 12:28 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 90
> > <https://reviews.apache.org/r/48182/diff/2/?file=1406146#file1406146line90>
> >
> > This is a test that Xinyu ported over from my experiments. Totally agreed that the names are not well-thought-out and I was mainly focusing on testing all RocksDB put/get/remove/iterator behavior under the synchronized lock. It turns out that RocksDB access is safe under the synchronized lock and this test probably is no longer needed.
+1. I don't think these tests are particularly useful as a part of the regular test suite.
- Chris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136735
-----------------------------------------------------------
On June 9, 2016, 12:33 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48182/
> -----------------------------------------------------------
>
> (Updated June 9, 2016, 12:33 a.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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136735
-----------------------------------------------------------
Ship it!
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala (line 90)
<https://reviews.apache.org/r/48182/#comment201834>
This is a test that Xinyu ported over from my experiments. Totally agreed that the names are not well-thought-out and I was mainly focusing on testing all RocksDB put/get/remove/iterator behavior under the synchronized lock. It turns out that RocksDB access is safe under the synchronized lock and this test probably is no longer needed.
- Yi Pan (Data Infrastructure)
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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Chris Pettitt <cp...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136879
-----------------------------------------------------------
Ship it!
Ship It!
- Chris Pettitt
On June 9, 2016, 12:33 a.m., Xinyu Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48182/
> -----------------------------------------------------------
>
> (Updated June 9, 2016, 12:33 a.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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Xinyu Liu <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/
-----------------------------------------------------------
(Updated June 9, 2016, 12:33 a.m.)
Review request for samza, Chris Pettitt, Navina Ramesh, and Yi Pan (Data Infrastructure).
Changes
-------
Updates on the unit tests based on Chris and Navina's feedback.
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 (updated)
-----
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
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Xinyu Liu <xi...@linkedin.com>.
-----------------------------------------------------------
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).
Changes
-------
Updates on Chris's feedback. Fixed the issues in unit tests.
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 (updated)
-----
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
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Xinyu Liu <xi...@linkedin.com>.
> On June 3, 2016, 5:49 p.m., Chris Pettitt wrote:
> > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala, lines 409-410
> > <https://reviews.apache.org/r/48182/diff/1/?file=1405222#file1405222line409>
> >
> > 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.
runner1 and runner2 can be running in any order. The result should be the same.
- Xinyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48182/#review136075
-----------------------------------------------------------
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
>
>
Re: Review Request 48182: SAMZA-958: Make store/cache thread safe
Posted by Chris Pettitt <cp...@linkedin.com>.
-----------------------------------------------------------
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
>
>