You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Naveen Somasundaram <na...@gmail.com> on 2015/05/01 01:21:21 UTC
Review Request 33735: RocksDB TTL support
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
RocksDB TTL support
https://issues.apache.org/jira/browse/SAMZA-537
https://issues.apache.org/jira/browse/SAMZA-442
Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
Diffs
-----
build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
Diff: https://reviews.apache.org/r/33735/diff/
Testing
-------
Added Unit test
Thanks,
Naveen Somasundaram
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82615
-----------------------------------------------------------
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
<https://reviews.apache.org/r/33735/#comment133369>
Makes sense, changed to rocksDbHandle
- Naveen Somasundaram
On April 30, 2015, 11:21 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated April 30, 2015, 11:21 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Yan Fang <ya...@gmail.com>.
> On May 7, 2015, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 94
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line94>
> >
> > It would be nicer to try catch the exception here and log an error, in case the db open failed.
also, not sure if RocksDB provides a way to detect if it's opened by TTL last time. If there is, maybe we should throw a log warning users when they disable the TTL after last open. If not, there's nothing we can do.
- Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82877
-----------------------------------------------------------
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
> On May 7, 2015, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, line 1010
> > <https://reviews.apache.org/r/33735/diff/2/?file=951492#file951492line1010>
> >
> > question: what is the default value for ttl here? null?
There is no default, if not specified, the store will open the store as a non-ttl. I think this follows the rest of the convention for docs, correct me if I am wrong :)
> On May 7, 2015, 8:07 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 94
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line94>
> >
> > It would be nicer to try catch the exception here and log an error, in case the db open failed.
>
> Yan Fang wrote:
> also, not sure if RocksDB provides a way to detect if it's opened by TTL last time. If there is, maybe we should throw a log warning users when they disable the TTL after last open. If not, there's nothing we can do.
Added exception catch! I checked Yan, there is no way to do that. Let me raise an issue with RocksDB, it would be nice to have.
- Naveen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82877
-----------------------------------------------------------
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82877
-----------------------------------------------------------
docs/learn/documentation/versioned/jobs/configuration-table.html
<https://reviews.apache.org/r/33735/#comment133714>
question: what is the default value for ttl here? null?
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
<https://reviews.apache.org/r/33735/#comment133731>
It would be nicer to try catch the exception here and log an error, in case the db open failed.
- Yi Pan (Data Infrastructure)
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 77
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line77>
> >
> > This kind of polymorphism should be accomplished via virtual methods; extend and override. The factory should inspect the config, and determine whether to return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl
I am not entirely sure I understand what is there to override here, what they share in common is the keyvalue interface, which is already abstracted out. If you are talking about abstracting out OpenDB itself and delegate it to a factory, the logic is too trivial to add a factory, at that point the factory will only do as much as just an if check.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, line 1009
> > <https://reviews.apache.org/r/33735/diff/2/?file=951492#file951492line1009>
> >
> > Please add the suffix ".ms":
> >
> > http://samza.apache.org/contribute/coding-guide.html#Configuration
> >
> > Configuration is the final third of our “UI”.
> > All configuration names that define time must end with .ms (e.g. foo.bar.ms=1000).
> >
> > I understand that the current implementation of the RocksDB TTL doesn't support granularity higher than seconds, but:
> > * we can convert, and round, to seconds (since TTL is not exact anyways)
> > * in the future if MS are supported, no need to change the contract here
I'll convert it to ms, rounding it to 1000 at a minimum.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 59
> > <https://reviews.apache.org/r/33735/diff/2/?file=951496#file951496line59>
> >
> > Why are we re-testing RocksDB's TTL here? Is there a way to query RocksDB to check whether or not the desired options were used when the DB was opened?
We are test to avoid regression, basically making sure we answer "what if the future release TTL doesn't work from Java ?, does "rocksdb.ttl.ms" config always work if any other change happens is the creation logic" we do the release, it's entirely possible that we upgrade the RocksDB version in our dependency and it breaks TTL. The latter is not supported AFAIK, lmk otherwise, I can integrate.
- Naveen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83438
-----------------------------------------------------------
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Mohamed Mahmoud (El-Geish)" <el...@gmail.com>.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 77
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line77>
> >
> > This kind of polymorphism should be accomplished via virtual methods; extend and override. The factory should inspect the config, and determine whether to return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl
>
> Naveen Somasundaram wrote:
> I am not entirely sure I understand what is there to override here, what they share in common is the keyvalue interface, which is already abstracted out. If you are talking about abstracting out OpenDB itself and delegate it to a factory, the logic is too trivial to add a factory, at that point the factory will only do as much as just an if check.
>
> Mohamed Mahmoud (El-Geish) wrote:
> What I meant is RocksDbKeyValueStorageEngineFactory::getKVStore should return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl instance based on the config; where the latter overrides OpenDB(). Today, the logic is a single if condition, but it opens the door for multiple cases in the future.
> I belive this is a really good use case of OOP here (especially the Open/Closed Principle); plus that's what factories are essentially for. RocksDbKeyValueStore doesn't have to worry about TTL-specific logic, and RocksDbKeyValueStoreWithTtl only cares about TTL (Single Responsibility Principle).
>
> Naveen Somasundaram wrote:
> I see what you are saying, but, "Today, the logic is a single if condition, but it opens the door for multiple cases in the future."
> When the logic gets that complicated, we'll introduce a factory :). Coding for the future, will only affect readability till we get there. Plus, if you look at it, it's not really a part of the RockDBKeyValueFactory itself, but rather a static method (mapping scala to java parlance). For now, I would like to stick to this.
XP; I can buy that.
- Mohamed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83438
-----------------------------------------------------------
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Mohamed Mahmoud (El-Geish)" <el...@gmail.com>.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 77
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line77>
> >
> > This kind of polymorphism should be accomplished via virtual methods; extend and override. The factory should inspect the config, and determine whether to return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl
>
> Naveen Somasundaram wrote:
> I am not entirely sure I understand what is there to override here, what they share in common is the keyvalue interface, which is already abstracted out. If you are talking about abstracting out OpenDB itself and delegate it to a factory, the logic is too trivial to add a factory, at that point the factory will only do as much as just an if check.
What I meant is RocksDbKeyValueStorageEngineFactory::getKVStore should return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl instance based on the config; where the latter overrides OpenDB(). Today, the logic is a single if condition, but it opens the door for multiple cases in the future.
I belive this is a really good use case of OOP here (especially the Open/Closed Principle); plus that's what factories are essentially for. RocksDbKeyValueStore doesn't have to worry about TTL-specific logic, and RocksDbKeyValueStoreWithTtl only cares about TTL (Single Responsibility Principle).
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 59
> > <https://reviews.apache.org/r/33735/diff/2/?file=951496#file951496line59>
> >
> > Why are we re-testing RocksDB's TTL here? Is there a way to query RocksDB to check whether or not the desired options were used when the DB was opened?
>
> Naveen Somasundaram wrote:
> We are test to avoid regression, basically making sure we answer "what if the future release TTL doesn't work from Java ?, does "rocksdb.ttl.ms" config always work if any other change happens is the creation logic" we do the release, it's entirely possible that we upgrade the RocksDB version in our dependency and it breaks TTL. The latter is not supported AFAIK, lmk otherwise, I can integrate.
I can see the blackbox methodology; I'm more of a graybox adovcate. I guess my concern here is the sleep, and the probablity of test failure if compaction wasn't triggered. If there's a way to trigger TTL immediately, that would be great.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, line 1009
> > <https://reviews.apache.org/r/33735/diff/2/?file=951492#file951492line1009>
> >
> > Please add the suffix ".ms":
> >
> > http://samza.apache.org/contribute/coding-guide.html#Configuration
> >
> > Configuration is the final third of our “UI”.
> > All configuration names that define time must end with .ms (e.g. foo.bar.ms=1000).
> >
> > I understand that the current implementation of the RocksDB TTL doesn't support granularity higher than seconds, but:
> > * we can convert, and round, to seconds (since TTL is not exact anyways)
> > * in the future if MS are supported, no need to change the contract here
>
> Naveen Somasundaram wrote:
> I'll convert it to ms, rounding it to 1000 at a minimum.
Thanks!
- Mohamed
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83438
-----------------------------------------------------------
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 77
> > <https://reviews.apache.org/r/33735/diff/2/?file=951495#file951495line77>
> >
> > This kind of polymorphism should be accomplished via virtual methods; extend and override. The factory should inspect the config, and determine whether to return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl
>
> Naveen Somasundaram wrote:
> I am not entirely sure I understand what is there to override here, what they share in common is the keyvalue interface, which is already abstracted out. If you are talking about abstracting out OpenDB itself and delegate it to a factory, the logic is too trivial to add a factory, at that point the factory will only do as much as just an if check.
>
> Mohamed Mahmoud (El-Geish) wrote:
> What I meant is RocksDbKeyValueStorageEngineFactory::getKVStore should return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl instance based on the config; where the latter overrides OpenDB(). Today, the logic is a single if condition, but it opens the door for multiple cases in the future.
> I belive this is a really good use case of OOP here (especially the Open/Closed Principle); plus that's what factories are essentially for. RocksDbKeyValueStore doesn't have to worry about TTL-specific logic, and RocksDbKeyValueStoreWithTtl only cares about TTL (Single Responsibility Principle).
I see what you are saying, but, "Today, the logic is a single if condition, but it opens the door for multiple cases in the future."
When the logic gets that complicated, we'll introduce a factory :). Coding for the future, will only affect readability till we get there. Plus, if you look at it, it's not really a part of the RockDBKeyValueFactory itself, but rather a static method (mapping scala to java parlance). For now, I would like to stick to this.
> On May 12, 2015, 6:10 p.m., Mohamed Mahmoud (El-Geish) wrote:
> > samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala, line 59
> > <https://reviews.apache.org/r/33735/diff/2/?file=951496#file951496line59>
> >
> > Why are we re-testing RocksDB's TTL here? Is there a way to query RocksDB to check whether or not the desired options were used when the DB was opened?
>
> Naveen Somasundaram wrote:
> We are test to avoid regression, basically making sure we answer "what if the future release TTL doesn't work from Java ?, does "rocksdb.ttl.ms" config always work if any other change happens is the creation logic" we do the release, it's entirely possible that we upgrade the RocksDB version in our dependency and it breaks TTL. The latter is not supported AFAIK, lmk otherwise, I can integrate.
>
> Mohamed Mahmoud (El-Geish) wrote:
> I can see the blackbox methodology; I'm more of a graybox adovcate. I guess my concern here is the sleep, and the probablity of test failure if compaction wasn't triggered. If there's a way to trigger TTL immediately, that would be great.
we do trigger compaction, which should be deterministic, the reason for the using the loop is, it's cleaner than doing a thread.sleep, which is less error prone as the compaction might not exactly align with the sleep.
- Naveen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83438
-----------------------------------------------------------
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Mohamed Mahmoud (El-Geish)" <el...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83438
-----------------------------------------------------------
docs/learn/documentation/versioned/jobs/configuration-table.html
<https://reviews.apache.org/r/33735/#comment134425>
Please add the suffix ".ms":
http://samza.apache.org/contribute/coding-guide.html#Configuration
Configuration is the final third of our “UI”.
All configuration names that define time must end with .ms (e.g. foo.bar.ms=1000).
I understand that the current implementation of the RocksDB TTL doesn't support granularity higher than seconds, but:
* we can convert, and round, to seconds (since TTL is not exact anyways)
* in the future if MS are supported, no need to change the contract here
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
<https://reviews.apache.org/r/33735/#comment134426>
This kind of polymorphism should be accomplished via virtual methods; extend and override. The factory should inspect the config, and determine whether to return a RocksDbKeyValueStore or a RocksDbKeyValueStoreWithTtl
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
<https://reviews.apache.org/r/33735/#comment134428>
Why are we re-testing RocksDB's TTL here? Is there a way to query RocksDB to check whether or not the desired options were used when the DB was opened?
- Mohamed Mahmoud (El-Geish)
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Mohamed Mahmoud (El-Geish)" <el...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83856
-----------------------------------------------------------
Ship it!
Ship It!
- Mohamed Mahmoud (El-Geish)
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Mohamed Mahmoud (El-Geish)" <el...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83688
-----------------------------------------------------------
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
<https://reviews.apache.org/r/33735/#comment134726>
floor vs. round -- thoughts?
- Mohamed Mahmoud (El-Geish)
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review83847
-----------------------------------------------------------
Ship it!
Ship It!
- Yi Pan (Data Infrastructure)
On May 13, 2015, 11:10 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 11:10 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/
-----------------------------------------------------------
(Updated May 13, 2015, 11:10 p.m.)
Review request for samza.
Changes
-------
Addressing review comments
Repository: samza
Description
-------
RocksDB TTL support
https://issues.apache.org/jira/browse/SAMZA-537
https://issues.apache.org/jira/browse/SAMZA-442
Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
Diffs (updated)
-----
build.gradle ac80a8664180e556ec83e229e04e3d8c56b70506
docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala dd20f171491da4b4d900551932b2a06d58526d73
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 9dee7be9a58c491dbd1a6b9cf73d5c111c570da2
Diff: https://reviews.apache.org/r/33735/diff/
Testing
-------
Added Unit test
Thanks,
Naveen Somasundaram
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/
-----------------------------------------------------------
(Updated May 6, 2015, 8:55 p.m.)
Review request for samza.
Repository: samza
Description
-------
RocksDB TTL support
https://issues.apache.org/jira/browse/SAMZA-537
https://issues.apache.org/jira/browse/SAMZA-442
Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
Diffs (updated)
-----
build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
Diff: https://reviews.apache.org/r/33735/diff/
Testing
-------
Added Unit test
Thanks,
Naveen Somasundaram
Re: Review Request 33735: RocksDB TTL support
Posted by Naveen Somasundaram <na...@gmail.com>.
> On May 1, 2015, 6:27 a.m., Yan Fang wrote:
> > build.gradle, line 34
> > <https://reviews.apache.org/r/33735/diff/1/?file=946534#file946534line34>
> >
> > remember to remove the space. :)
I have published it to maven, getting rid of this :)
> On May 1, 2015, 6:27 a.m., Yan Fang wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, lines 1009-1010
> > <https://reviews.apache.org/r/33735/diff/1/?file=946535#file946535line1009>
> >
> > why do we remove the bloomfilter?
Apparently this is only supported in https://github.com/facebook/rocksdb/wiki/PlainTable-Format. We use the block based format, we can't use plain table format because it doesn't support iterator.prev().
> On May 1, 2015, 6:27 a.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala, line 49
> > <https://reviews.apache.org/r/33735/diff/1/?file=946537#file946537line49>
> >
> > do we follow the same name fashion? using rocksDbHandle instead?
Makes sense, fixed.
> On May 1, 2015, 6:27 a.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, lines 78-80
> > <https://reviews.apache.org/r/33735/diff/1/?file=946538#file946538line78>
> >
> > 1. throw Exception when the value is not int?
> > 2. add warning/info messages for this setting. Because if this is set too low accidently, at least we have a place to check.
1. Done
2. Done
- Naveen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82237
-----------------------------------------------------------
On April 30, 2015, 11:21 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated April 30, 2015, 11:21 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Yan Fang <ya...@gmail.com>.
> On May 1, 2015, 6:27 a.m., Yan Fang wrote:
> > docs/learn/documentation/versioned/jobs/configuration-table.html, lines 1009-1010
> > <https://reviews.apache.org/r/33735/diff/1/?file=946535#file946535line1009>
> >
> > why do we remove the bloomfilter?
>
> Naveen Somasundaram wrote:
> Apparently this is only supported in https://github.com/facebook/rocksdb/wiki/PlainTable-Format. We use the block based format, we can't use plain table format because it doesn't support iterator.prev().
Does this mean, previously, the bloomfilter actually was just for decorating? useless?
- Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82237
-----------------------------------------------------------
On May 6, 2015, 8:55 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated May 6, 2015, 8:55 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>
Re: Review Request 33735: RocksDB TTL support
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33735/#review82237
-----------------------------------------------------------
build.gradle
<https://reviews.apache.org/r/33735/#comment132933>
remember to remove the space. :)
docs/learn/documentation/versioned/jobs/configuration-table.html
<https://reviews.apache.org/r/33735/#comment132934>
why do we remove the bloomfilter?
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala
<https://reviews.apache.org/r/33735/#comment132935>
do we follow the same name fashion? using rocksDbHandle instead?
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
<https://reviews.apache.org/r/33735/#comment132936>
1. throw Exception when the value is not int?
2. add warning/info messages for this setting. Because if this is set too low accidently, at least we have a place to check.
- Yan Fang
On April 30, 2015, 11:21 p.m., Naveen Somasundaram wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33735/
> -----------------------------------------------------------
>
> (Updated April 30, 2015, 11:21 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB TTL support
> https://issues.apache.org/jira/browse/SAMZA-537
> https://issues.apache.org/jira/browse/SAMZA-442
>
> Please ignore the maven link added to build.gradle, I'll remove it once I validate the release is good.
>
>
> Diffs
> -----
>
> build.gradle ebad6eb27372d13bdb76506db2d4372b128f3c1e
> docs/learn/documentation/versioned/jobs/configuration-table.html 728197d01d1e3f551ea53e2a14e97df44e29ee19
> gradle/dependency-versions.gradle ee6dfc411b7ab90b187df79f109884127953862e
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStorageEngineFactory.scala 5ab68590a4ed2686d730344665e25776cade6add
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala 66c2a0dc2e38e21f951727a30f0987776ac52fe2
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala PRE-CREATION
> samza-test/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala 50dfc10bb053d74dba70fdbce0ef87609ba447ea
>
> Diff: https://reviews.apache.org/r/33735/diff/
>
>
> Testing
> -------
>
> Added Unit test
>
>
> Thanks,
>
> Naveen Somasundaram
>
>