Re: Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-20 Thread tao feng 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/
---

(Updated Nov. 20, 2015, 8:20 a.m.)


Review request for samza.


Repository: samza


Description (updated)
---

1. implement RocksDB flush method;
2. RocksDB flushOptions waitForFlush set to true;
3. unit test
 
Diff-2: Update per Yi and Navina's comments for unit test: open a read only 
RocksDB to verify the data


Diffs (updated)
-

  
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
 b949793a63951576937fa848bd674ec68f6f9727 
  
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
 4620037f2a0da43149a754aa808d3d5d280ea893 
  
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
 a428a16bc1e9ab4980a6f17db4fd810057d31136 

Diff: https://reviews.apache.org/r/40525/diff/


Testing
---

1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest


Thanks,

tao feng

Re: Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-20 Thread Yi Pan (Data Infrastructure) 


> On Nov. 20, 2015, 6:53 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala,
> >  line 84
> > 
> >
> > To make sure that it is flush() that write to the disk, not close(), 
> > you may want to keep this db open and open another db in read-only mode to 
> > verify that the read-only db sees the data.
> 
> tao feng wrote:
> I dont think Samza-RocksDB supports read-only open mode. I directly use 
> RocksDB interface here to do the verification. Let me know if it is not a 
> good practice here.

Check RocksDbKeyValueReader.java. It has implemented the way to open a RocksDb 
instance with read-only options.


- Yi


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/#review107320
---


On Nov. 20, 2015, 8:20 a.m., tao feng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40525/
> ---
> 
> (Updated Nov. 20, 2015, 8:20 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1. implement RocksDB flush method;
> 2. RocksDB flushOptions waitForFlush set to true;
> 3. unit test
>  
> Diff-2: Update per Yi and Navina's comments for unit test: open a read only 
> RocksDB to verify the data
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
>  b949793a63951576937fa848bd674ec68f6f9727 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  4620037f2a0da43149a754aa808d3d5d280ea893 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/40525/diff/
> 
> 
> Testing
> ---
> 
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 
> 
> Thanks,
> 
> tao feng
> 
>

Re: Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-19 Thread Jagadish Venkatraman 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/#review107319
---

Ship it!


lgtm

- Jagadish Venkatraman


On Nov. 20, 2015, 6:18 a.m., tao feng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40525/
> ---
> 
> (Updated Nov. 20, 2015, 6:18 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1. implement RocksDB flush method; 
> 2. RocksDB flushOptions waitForFlush set to true; 
> 3. unit test
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
>  b949793a63951576937fa848bd674ec68f6f9727 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  4620037f2a0da43149a754aa808d3d5d280ea893 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/40525/diff/
> 
> 
> Testing
> ---
> 
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 
> 
> Thanks,
> 
> tao feng
> 
>

Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-19 Thread tao feng 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/
---

Review request for samza.


Repository: samza


Description
---

1. implement RocksDB flush method; 
2. RocksDB flushOptions waitForFlush set to true; 
3. unit test


Diffs
-

  
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
 b949793a63951576937fa848bd674ec68f6f9727 
  
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
 4620037f2a0da43149a754aa808d3d5d280ea893 
  
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
 a428a16bc1e9ab4980a6f17db4fd810057d31136 

Diff: https://reviews.apache.org/r/40525/diff/


Testing
---

1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest


Thanks,

tao feng

Re: Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-19 Thread Yi Pan (Data Infrastructure) 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/#review107320
---



samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
 (line 84)


To make sure that it is flush() that write to the disk, not close(), you 
may want to keep this db open and open another db in read-only mode to verify 
that the read-only db sees the data.


- Yi Pan (Data Infrastructure)


On Nov. 20, 2015, 6:18 a.m., tao feng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40525/
> ---
> 
> (Updated Nov. 20, 2015, 6:18 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1. implement RocksDB flush method; 
> 2. RocksDB flushOptions waitForFlush set to true; 
> 3. unit test
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
>  b949793a63951576937fa848bd674ec68f6f9727 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  4620037f2a0da43149a754aa808d3d5d280ea893 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/40525/diff/
> 
> 
> Testing
> ---
> 
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 
> 
> Thanks,
> 
> tao feng
> 
>

Re: Review Request 40525: SAMZA-819: RocksDbKeyValueStore.flush() should be implemented

2015-11-19 Thread Navina Ramesh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40525/#review107321
---

Ship it!



samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
 (line 84)


I agree. Calling close() doesn't really test the case.


- Navina Ramesh


On Nov. 20, 2015, 6:18 a.m., tao feng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40525/
> ---
> 
> (Updated Nov. 20, 2015, 6:18 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> 1. implement RocksDB flush method; 
> 2. RocksDB flushOptions waitForFlush set to true; 
> 3. unit test
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
>  b949793a63951576937fa848bd674ec68f6f9727 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  4620037f2a0da43149a754aa808d3d5d280ea893 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/40525/diff/
> 
> 
> Testing
> ---
> 
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 
> 
> Thanks,
> 
> tao feng
> 
>