----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26059/#review54688 -----------------------------------------------------------
build.gradle <https://reviews.apache.org/r/26059/#comment94929> We aren't using grizzled any more. Have you rebased against master? Also, I'd recommend installing rbt, so you can just do rbt post rather than manually uploading RBs. samza-kv-leveldb/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala <https://reviews.apache.org/r/26059/#comment94930> Nit: can we just do one-per-line here. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala <https://reviews.apache.org/r/26059/#comment94931> License header. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala <https://reviews.apache.org/r/26059/#comment94933> Nit: single line spacing. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala <https://reviews.apache.org/r/26059/#comment94932> Nit: single line spacing. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala <https://reviews.apache.org/r/26059/#comment94938> We should delete this. It was a hack for LevelDB's .all() latency issues that we were seeing. It didn't really seem to work all that well. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94955> License. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94939> Should be able to do "RocksDbKeyValueStore with Logging", and then just call warn(). samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94942> Do we need to support the other compressions that RocksDB supports (e.g. bzip2)? samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94943> Seems like these should be configurable: compaction style, filter, write buff num, cache size. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94945> Delete. See comment above. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94946> Nit: options, dir.toString (space) samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94947> Nit: remove double space. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94948> because of? samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94949> Is this true? I know it's not supported for LevelDB. Wonder if RocksDB added it? samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94950> Remove //iter.remove(); samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94951> Clean this up. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94952> This looks like it was cargo culted in from LevelDB's implementation. Can we put this in one place, and share between the two? Either samza-kv or samza-core's util. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala <https://reviews.apache.org/r/26059/#comment94953> Don't need these. They're from DBComparator, which is LevelDB's class. Seems we should make LexicographicaCompartor a shared class, and have LevelDB extend it with DBCompartor to add these two methods. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/TestRocksDbApi.scala <https://reviews.apache.org/r/26059/#comment94954> License. samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/TestRocksDbApi.scala <https://reviews.apache.org/r/26059/#comment94956> Docs. Move to samza-test. I've been trying to keep CLI-based test tools isolated there. Alternatively, re-write as a Unit test. - Chris Riccomini On Sept. 25, 2014, 11:17 p.m., Naveen Somasundaram wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26059/ > ----------------------------------------------------------- > > (Updated Sept. 25, 2014, 11:17 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-236 > https://issues.apache.org/jira/browse/SAMZA-236 > > > Repository: samza > > > Description > ------- > > Adding RocksDB Key-value support > > > Diffs > ----- > > build.gradle 357bb0e312c3812b55303847163f696f7c52ad78 > > samza-kv-leveldb/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala > d6d437ab7a8b18002dbeb54d85c6c2b5eb43784c > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala > PRE-CREATION > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala > PRE-CREATION > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/TestRocksDbApi.scala > PRE-CREATION > > samza-test/src/main/scala/org/apache/samza/test/performance/TestKeyValuePerformance.scala > 7d0b8db0c3bf1e70fd6af03abb594c7000e6666b > settings.gradle 325cac27389093d87eb9e36d475e234ed52510ec > > Diff: https://reviews.apache.org/r/26059/diff/ > > > Testing > ------- > > Unit testing > > > Thanks, > > Naveen Somasundaram > >
