----------------------------------------------------------- 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 > >
