> On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > Overall, looks pretty good to me. > > > > I think you had mentioned you had some thoughts on refactoring the > > getStorageEngine API. Are you planning on a follow-on JIRA for that?
Yes, I'll open a new ticket for that. > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > build.gradle, line 77 > > <https://reviews.apache.org/r/23372/diff/1/?file=627052#file627052line77> > > > > If memory requires guava, can we move it into it out of core? Moved to samza-kv-inmemory > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala, > > line 1 > > <https://reviews.apache.org/r/23372/diff/1/?file=627054#file627054line1> > > > > License header. Will do. > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > samza-core/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala, > > line 13 > > <https://reviews.apache.org/r/23372/diff/1/?file=627054#file627054line13> > > > > Javadoc. Will Do. > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LevelDbKeyValueStoreMetrics.scala, > > line 27 > > <https://reviews.apache.org/r/23372/diff/1/?file=627066#file627066line27> > > > > This seems like a copy/paste of LevelDbKeyValueStoreMetrics. Can we > > just move this into core as a generalized metrics class? > > > > Going one step further, we could make a wrapper around the KV store in > > BaseKeyValueStorageEngineFactory, which would handle these metrics for all > > underlying KV stores. I imagine we'll want these metrics whether it's in > > mem, level db, or rocksdb. Moved it to samza-kv > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > samza-kv/src/main/scala/org/apache/samza/storage/kv/NullSafeKeyValueStore.scala, > > line 32 > > <https://reviews.apache.org/r/23372/diff/1/?file=627070#file627070line32> > > > > Do we need the fully qualified path? Cleaned up (I blame IntelliJ for doing this :) ) > On July 9, 2014, 7:28 p.m., Chris Riccomini wrote: > > samza-kv/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala, > > line 42 > > <https://reviews.apache.org/r/23372/diff/1/?file=627073#file627073line42> > > > > Could you doc a quick note on what these params are now? It was easy > > enough when we only tested LDB, but with in mem, I'm a bit confused at > > first glance. Done - Chinmay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23372/#review47505 ----------------------------------------------------------- On July 9, 2014, 5:47 p.m., Chinmay Soman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/23372/ > ----------------------------------------------------------- > > (Updated July 9, 2014, 5:47 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > SAMZA-256: Provide in-memory data store implementation > > > Diffs > ----- > > build.gradle f1a458b8c4cbe6e38f28ba3c5bd9099590c7abde > gradle/dependency-versions-scala-2.10.gradle > 7094d6cf86b9da454633525dc5da65e5a9f90f4c > > samza-core/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala > PRE-CREATION > > samza-core/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala > PRE-CREATION > samza-kv/src/main/java/org/apache/samza/storage/kv/Entry.java > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueIterator.java > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala > 429f51a4d269e3d18627c183359af672c68b8b00 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStoreMetrics.scala > 8ffcfb194b981a798fd19b05f9e6bff9fc2f79e2 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStoreMetrics.scala > 8ffcfb194b981a798fd19b05f9e6bff9fc2f79e2 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala > f42ea026cd4a2ae633d71149af6dacef68dab352 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineFactory.scala > 36b7e97b427d3d0fd18f38609e2dac2b081a8dbc > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineMetrics.scala > a7958f62582a2cc998f026c6818afd24936d46d4 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LevelDbKeyValueStore.scala > dae3c2c8570d4add7bc3c01f0fb73c867c609740 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LevelDbKeyValueStoreMetrics.scala > 8949f2fb6d8488e5e061139299e9ea97220ea180 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LevelDbKeyValueStoreMetrics.scala > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStore.scala > d3c1ae84b56827964ce10d2aa9dedcdc7221c596 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStoreMetrics.scala > ea56e8c2e41efa0c59bedac9e489b399fc8813e4 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/NullSafeKeyValueStore.scala > 00f9af319c780d9513b735016b1c3e016714f5c8 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala > 2d3b6e55c20172aa5f149c6d6728cab8229312c3 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStoreMetrics.scala > 2ad21c822f3d9c08dcc1e78d4b86c3fd314b7ed2 > > samza-kv/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala > ec23567570e2d7868390f570bd65d8cd7b18d4dc > > samza-leveldb/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineFactory.scala > PRE-CREATION > > samza-test/src/main/scala/org/apache/samza/test/performance/TestKeyValuePerformance.scala > 0077af08ac33bd95b1d40c362f95dfd6208cf99c > settings.gradle 6ba628059a3168abf80d072d68942f79bd237811 > > Diff: https://reviews.apache.org/r/23372/diff/ > > > Testing > ------- > > > Thanks, > > Chinmay Soman > >
