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