Re: Review Request 36973: SAMZA-626: tool to read the RocksDb in a running job

["Yi Pan (Data Infrastructure)" <yi...@linkedin.com>]

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