samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Navina Ramesh" <nram...@linkedin.com>
Subject Re: Review Request 39252: SAMZA-626 - tool to read the RocksDb in a running job (Yan's patch)
Date Fri, 23 Oct 2015 18:25:51 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message