----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36973/#review101281 -----------------------------------------------------------
Overall looks good to me. Thanks! samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala (line 69) <https://reviews.apache.org/r/36973/#comment158660> nit: remove the whitespaces samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueReader.java (line 64) <https://reviews.apache.org/r/36973/#comment158661> nit: I don't see those two variables to be used multiple times. Maybe merged w/: keySerde = getSerdeFromName(storageConfig.getStorageKeySerde(storeName), serializerConfig) samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueReader.java (line 73) <https://reviews.apache.org/r/36973/#comment158662> nit: trailing white space samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueReader.java (line 87) <https://reviews.apache.org/r/36973/#comment158663> I don't think that this convenient constructor will buy us much. We often see the cases that users usually configure multiple state stores in a job. samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueReader.java (line 139) <https://reviews.apache.org/r/36973/#comment158664> Similarly, I don't think that this will buy us much. samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueStoreHelper.java (line 39) <https://reviews.apache.org/r/36973/#comment158665> nit: remove the whitespaces. samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueStoreHelper.java (line 66) <https://reviews.apache.org/r/36973/#comment158667> It would be clearer as the following: log.warn("Unknown rocksdb.compression codec " + compressionInConfig + ", overwriting to Snappy"); samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbKeyValueStoreHelper.java (line 90) <https://reviews.apache.org/r/36973/#comment158668> same here. samza-kv-rocksdb/src/main/java/org/apache/samza/storage/kv/RocksDbReadingTool.java (line 45) <https://reviews.apache.org/r/36973/#comment158669> I would prefer to keep this as required arg. - Yi Pan (Data Infrastructure) On July 31, 2015, 3:11 a.m., Yan Fang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36973/ > ----------------------------------------------------------- > > (Updated July 31, 2015, 3:11 a.m.) > > > Review request for samza. > > > Bugs: SAMZA-626 > https://issues.apache.org/jira/browse/SAMZA-626 > > > Repository: samza > > > Description > ------- > > *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 > > > Diffs > ----- > > build.gradle 0852adc > docs/learn/documentation/versioned/container/state-management.md 50d4b65 > samza-core/src/main/java/org/apache/samza/config/JavaSerializerConfig.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/config/JavaStorageConfig.java > af7d4ca > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 27b2517 > samza-core/src/main/scala/org/apache/samza/util/Util.scala 419452c > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > 84fdeaa > samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala ead6f94 > > 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 > 571a50e > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala > a423f7b > > 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/36973/diff/ > > > Testing > ------- > > > Thanks, > > Yan Fang > >
