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