> On May 5, 2016, 4:18 p.m., Jake Maes wrote: > > samza-api/src/main/java/org/apache/samza/storage/StorageEngine.java, line 62 > > <https://reviews.apache.org/r/46644/diff/1/?file=1360200#file1360200line62> > > > > Since there is a change to the API, we probably shouldn't commit this > > until after the 10.1 release, for the sake of semantic versioning.
Yeah I agree. Strictly speaking, we shouldn't introduce api changes in bug-fix version. But this patch is a "bug-fix" that is better solved by introducing a change to the interface. That said, I don't believe there will be any users implementing custom "StorageEngine". If at all, there may be custom implementations of KeyValueStores. Given our release timelines and the fact that this bug needs to be addressed asap, I think we should document the interface change and continue with the patch. > On May 5, 2016, 4:18 p.m., Jake Maes wrote: > > samza-api/src/main/java/org/apache/samza/storage/StoreProperties.java, line > > 22 > > <https://reviews.apache.org/r/46644/diff/1/?file=1360202#file1360202line22> > > > > Javadoc missing everywhere in this file except here. > > > > For example, will the cacheEnabled and loggedStore flags only be true > > for the CachedStore and LoggedStore (sub)classes, or do they have a > > different meaning? > > > > Is a cached store one that employs any kind of cache, or only a cache > > that might require flushes to disk? > > > > I assume loggedstores are those that have changelog enabled, but to > > newcomers that may not be obvious. I will add the Javadocs. The properties in StoreProperties are a collective set of properties for a given store. Not a particular subclass. By cacheEnabled, I only meant to imply whether the "CachedStored" wrapper is enabled or not. However, as Chris pointed out, that is not used anywhere. So, I am removing it from the list of properties. - Navina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46644/#review131844 ----------------------------------------------------------- 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 > >
