You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chris Pettitt <cp...@linkedin.com> on 2016/05/05 15:22:29 UTC
Re: Review Request 46644: SAMZA-889 - Change log not working properly
with In memory Store
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46644/#review131835
-----------------------------------------------------------
Fix it, then Ship it!
Looks good. I'd suggest moving non-change-related code to a separate cleanup RB.
samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java (line 29)
<https://reviews.apache.org/r/46644/#comment195875>
Generally you make the constructor private, but this is such a trivial value object that I wouldn't have a strong opinion either way.
samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java (lines 31 - 33)
<https://reviews.apache.org/r/46644/#comment195873>
Prefer boolean to Boolean through out. Boolean is a boxed type (thus nullable), introducing a third state whereas boolean has only two states to manage.
samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java (line 40)
<https://reviews.apache.org/r/46644/#comment195879>
It would be great to have some documentation about what each of these properties means. It's probably already documented somewhere, so a javadoc link might be sufficient.
samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java (lines 43 - 45)
<https://reviews.apache.org/r/46644/#comment195876>
What is this for? It doesn't appear to be used. Generally bias towards not adding things that are unused ceteris paribus.
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala (line 18)
<https://reviews.apache.org/r/46644/#comment195880>
Generally it is a best practice to seperate out non-change-related clean up to a separate commit. This helps when working with history (e.g. git bisect).
- Chris Pettitt
On April 25, 2016, 5:25 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46644/
> -----------------------------------------------------------
>
> (Updated April 25, 2016, 5:25 p.m.)
>
>
> Review request for samza, Boris Shkolnik, Chris Pettitt, and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding StoreProperties that is accessible from the StorageEngine
>
> Added a lifecycle unit test for TaskStorageManager
>
> Question: Is there a better way to refactor the TaskStorageManager class?
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml c15b8e74de8e5aac5ac83278c52ab3dba1630e50
> samza-api/src/main/java/org/apache/samza/storage/StorageEngine.java 5463648fd01e0cba52fa9bd9a33b247e7014cfde
> samza-api/src/main/java/org/apache/samza/storage/StorageEngineFactory.java adb62643a311e25fb4fed91c39e1a75cd5664b17
> samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 5462208c08cddbfd30d886daffa8c02c82447059
> samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala c7b05203a1958a62af9dec04b215d985c4646dc4
> samza-core/src/test/java/org/apache/samza/storage/MockStorageEngine.java b90ea87b7e575e646c58ddfb5a53ced9ed04a880
> samza-core/src/test/java/org/apache/samza/storage/MockStorageEngineFactory.java c00c4547307f5a8b401c6bb6438eaa7fb8a38651
> samza-core/src/test/java/org/apache/samza/storage/TestStorageRecovery.java 13f4fa97d42b02e54634c8de1575118ca0433fe8
> samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala c8ea64c7c67dd6bf789d2a3445d620ccef1beac0
> samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStorageEngineFactory.scala 53147ad4faae2fae24a5bc0677167d06c64afead
> samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala 661a83517c5a603c841d4a373ac979724457471c
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala dae6e35d1ba75daf5c816bccbc625c623a44d3b2
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala b896810ccf7f12d72195f07cac27ba5cc510077d
> samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala 391cf89b05f90ececae63160cd3cb9c811e4ab66
> samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala 4c4e82eb5be82d469fe3c5f85b92523faeb0a193
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala defc91e7e28b1d9419b98f363f9989d58511e923
> samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStore.scala e293bfc54fda5e302c22cc85b8fee384d2fe8f35
> samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala cf1a2cc16a5c2fb547012359a32b22c207d64d8e
> samza-kv/src/test/scala/org/apache/samza/storage/kv/MockKeyValueStore.scala e14a4617ef23b2ae7b89aee93219677e29455d6c
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 23f8a1a6bee8ef38e0640a4e90778e53d982deeb
>
> Diff: https://reviews.apache.org/r/46644/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
> TODO: Test with sample job to verify that the behavior reported in SAMZA-889 is fixed
>
>
> Thanks,
>
> Navina Ramesh
>
>
Re: Review Request 46644: SAMZA-889 - Change log not working properly
with In memory Store
Posted by Navina Ramesh <nr...@linkedin.com>.
> On May 5, 2016, 3:22 p.m., Chris Pettitt wrote:
> > samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java, line 40
> > <https://reviews.apache.org/r/46644/diff/1/?file=1360202#file1360202line40>
> >
> > It would be great to have some documentation about what each of these properties means. It's probably already documented somewhere, so a javadoc link might be sufficient.
Done!
> On May 5, 2016, 3:22 p.m., Chris Pettitt wrote:
> > samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java, lines 43-45
> > <https://reviews.apache.org/r/46644/diff/1/?file=1360202#file1360202line43>
> >
> > What is this for? It doesn't appear to be used. Generally bias towards not adding things that are unused ceteris paribus.
I added this property for completeness in the properties we may need to set. However, you are right. Not worth introducing something which we don't currently use.
> On May 5, 2016, 3:22 p.m., Chris Pettitt wrote:
> > samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala, line 18
> > <https://reviews.apache.org/r/46644/diff/1/?file=1360219#file1360219line18>
> >
> > Generally it is a best practice to seperate out non-change-related clean up to a separate commit. This helps when working with history (e.g. git bisect).
I agree.But we have so much of debt that it never gets cleaned up. Some of them were IDEA imposed auto-corrects. I have disabled them. I will try to keep the non-change-related code separate going forward. Thanks for the guidance! :)
- Navina
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46644/#review131835
-----------------------------------------------------------
On May 5, 2016, 9:13 p.m., Navina Ramesh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46644/
> -----------------------------------------------------------
>
> (Updated May 5, 2016, 9:13 p.m.)
>
>
> Review request for samza, Boris Shkolnik, Chris Pettitt, and Yi Pan (Data Infrastructure).
>
>
> Repository: samza
>
>
> Description
> -------
>
> Adding StoreProperties that is accessible from the StorageEngine
>
> Added a lifecycle unit test for TaskStorageManager
>
> Question: Is there a better way to refactor the TaskStorageManager class?
>
>
> Diffs
> -----
>
> checkstyle/import-control.xml 9afab881f2a822925ae716e1dbd744b86321c34e
> samza-api/src/main/java/org/apache/samza/storage/StorageEngine.java 5463648fd01e0cba52fa9bd9a33b247e7014cfde
> samza-api/src/main/java/org/apache/samza/storage/StorageEngineFactory.java adb62643a311e25fb4fed91c39e1a75cd5664b17
> samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java PRE-CREATION
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala 5462208c08cddbfd30d886daffa8c02c82447059
> samza-core/src/main/scala/org/apache/samza/storage/TaskStorageManager.scala c7b05203a1958a62af9dec04b215d985c4646dc4
> samza-core/src/test/java/org/apache/samza/storage/MockStorageEngine.java b90ea87b7e575e646c58ddfb5a53ced9ed04a880
> samza-core/src/test/java/org/apache/samza/storage/MockStorageEngineFactory.java c00c4547307f5a8b401c6bb6438eaa7fb8a38651
> samza-core/src/test/java/org/apache/samza/storage/TestStorageRecovery.java 13f4fa97d42b02e54634c8de1575118ca0433fe8
> samza-core/src/test/scala/org/apache/samza/storage/TestTaskStorageManager.scala c8ea64c7c67dd6bf789d2a3445d620ccef1beac0
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala dae6e35d1ba75daf5c816bccbc625c623a44d3b2
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala f0965aec5f3ec2a214dc40c70832c58273623749
> samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala 391cf89b05f90ececae63160cd3cb9c811e4ab66
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala e5a66a4770b9553a1cc48fbb505f52d123c6c754
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 23f8a1a6bee8ef38e0640a4e90778e53d982deeb
>
> Diff: https://reviews.apache.org/r/46644/diff/
>
>
> Testing
> -------
>
> ./gradlew clean build
> TODO: Test with sample job to verify that the behavior reported in SAMZA-889 is fixed
>
>
> Thanks,
>
> Navina Ramesh
>
>