> On Oct. 23, 2015, 1:22 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueStoreHelper.java, > > line 34 > > <https://reviews.apache.org/r/39252/diff/2/?file=1100420#file1100420line34> > > > > nit: maybe rename to RocksDbOptionsHelper, to be more accurate stating > > the function of this class?
Makes sense > On Oct. 23, 2015, 1:22 a.m., Yi Pan (Data Infrastructure) wrote: > > docs/learn/documentation/versioned/container/state-management.md, line 214 > > <https://reviews.apache.org/r/39252/diff/2/?file=1100412#file1100412line214> > > > > From the code, it seems that the argument is required. We should fix > > the document here. Fixed > On Oct. 23, 2015, 1:22 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbReadingTool.java, > > line 80 > > <https://reviews.apache.org/r/39252/diff/2/?file=1100421#file1100421line80> > > > > Shouldn't we validate only one keyArgu is included in the options? I think we overwrite the key variable each time. So, only the last sepcified key will be considered. Should we still validate that only 1 type is provided? > On Oct. 23, 2015, 1:22 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbReadingTool.java, > > line 96 > > <https://reviews.apache.org/r/39252/diff/2/?file=1100421#file1100421line96> > > > > nit: Since the db-path and db-name are required options, the if > > conditions seem to be unnecessary. Yeah. I have to fail on else condition because db-name and db-path are required arguments. Will fix it. > On Oct. 23, 2015, 1:22 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/config/JavaSerializerConfig.java, > > line 1 > > <https://reviews.apache.org/r/39252/diff/2/?file=1100413#file1100413line1> > > > > I noticed that we are creating new JavaSerializerConfig, > > JavaStorageConfig, etc. instead of change the scala classes directly to > > Java. Why is it? It seems to be a good opportunity to move part of the code > > to Java. Yeah. I think we create Java config classes because we want to move away from config implicits and in general, scala code. Ideally, though we should get rid of the corresponding scala config class when moving to java so that we don't have the overhead of maintaining 2 classes. This requires changes in other parts of the code. I will work on that now! - Navina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39252/#review103702 ----------------------------------------------------------- On Oct. 16, 2015, 11:44 p.m., Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39252/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2015, 11:44 p.m.) > > > Review request for samza, Yan Fang, Chinmay Soman, Jake Maes, Yi Pan (Data > Infrastructure), and Jagadish Venkatraman. > > > Bugs: SAMZA-626 > https://issues.apache.org/jira/browse/SAMZA-626 > > > Repository: samza > > > Description > ------- > > Porting changes from Yan's patch in https://reviews.apache.org/r/36973/ > > *refactored some java code > > *changed RocksDbKeyValueStore.options form scala to java > > *moved default serde name from container to util, because it is useful to > other classes > > *added a class to read the running rocksdb > > *added a commondline tool > > *updated the doc accordingly > > https://reviews.apache.org/r/36973/#issue-summary > > > Diffs > ----- > > build.gradle 682d4f80f33939d8471c9f0cecb7ccbf4eb1bfec > docs/learn/documentation/versioned/container/state-management.md > 50d4b657582661975369c1caa088d9b8b55d7745 > samza-core/src/main/java/org/apache/samza/config/JavaSerializerConfig.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/config/JavaStorageConfig.java > af7d4ca70f77eb4865b52fe71a55f506b60474e7 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > f351ad6959eadde8766140bd3e6334a18bda4886 > samza-core/src/main/scala/org/apache/samza/util/Util.scala > 948c19ab39ff1686be4efe6a55a6fc9aa6a01d86 > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > 6de8710d5a4ebceca3df6cd925388441f8823a37 > samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala > d16726393d844b3de7a814db2a40f71b8feac3dc > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueReader.java > PRE-CREATION > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueStoreHelper.java > PRE-CREATION > > samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbReadingTool.java > PRE-CREATION > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala > 571a50e4a9abee25e880db3c268ccf892f2c5125 > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala > a423f7bd6c43461e051b5fd1f880dd01db785991 > > samza-kv-rocksdb/src/test/java/org/apache/samza/storage/kv/TestRocksDbKeyValueReader.java > PRE-CREATION > samza-shell/src/main/bash/read-rocksdb-tool.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/39252/diff/ > > > Testing > ------- > > ./bin/check-all.sh > Tested and verified with sample job on Yarn. > > > Thanks, > > Navina Ramesh > >
