----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41068/#review109417 -----------------------------------------------------------
@Amit, thanks for putting up this patch. I have two high-level comments: * I would prefer to keep the return type of seek() as an iterator. The pattern of the current API looks fine to me: all() and range() gives iterators. All it needs is to add seek() API, which also gives an iterator as a returned value which starts w/ certain position. With that, seekToFirst() and newIterator() are not needed. And what's the particular use case for seekToLast()? It does not seem to be useful. Do you have any specific use case for it? It would also appears more natural if we just add APIs like: first(), last(), instead of seekToLast(). Thanks! samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala (line 67) <https://reviews.apache.org/r/41068/#comment168919> nit: the doc does not seem to be complete samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala (line 69) <https://reviews.apache.org/r/41068/#comment168922> Won't all three operations create a new iterator? I think that using TreeMap, we can create iterators for that purpose. Also, what's the purpose to have seekToFirst() and seekToLast() in the APIs? Let's assume that the seek*() methods all return an iterator as the return value. seekToFirst() is equivalent to all(), seek() is similar but not exactly range(). And what's seekToLast() serves? It seems to be that the only thing needed to be added here is seek(), which provides a semi-close range query that the existing range() does not support. samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala (line 88) <https://reviews.apache.org/r/41068/#comment168924> Why do we need to have an explicit method to create newIterator()? We can have seek() creates the iterator and returns it, which is matching the pattern with all the other APIs in KV-store - Yi Pan (Data Infrastructure) On Dec. 8, 2015, 1:47 a.m., Amit Yadav wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41068/ > ----------------------------------------------------------- > > (Updated Dec. 8, 2015, 1:47 a.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Added Seek functionality to KeyValueStore and KeyValueStoreIterator. The > following alternatives were considered > > 1. Create a 'SeekingKeyValueIterator' and 'SeekingKeyValueStore' - Have a > completely separate classes which extends their existing > counterparts. This solution is appealing as it provides separation of > concerns. However, this also requires creating a complete > hierarchy of all the classes which are implementing the > KeyValueStore/Iterator interface. Therefore the effort to value didnt > made much sense. > > 2. Use the existing 'range' functionality - This solution requires a new > iterator to be created which could supply the 'end' > key for the range. We could make this user configurable, however this > assumes that the user can somehow come up with an > end key in all the cases. This assumption may not be true all the times. > Also, I feel this is more of a workaround then > a real solution, more of range degenerated to a seekTo functionality and > therefore is not a clean interaface. > > 3. Add seek functionality to existing interfaces and classes :- In the end, > this is what I implemented since it is the cleanest solution > in amongst all the options. This also requires minimum changes. The > downside of this approach is that not al stores can support seek > to a particular key (especially, the one derived from java iterators). > > > Diffs > ----- > > > samza-kv-inmemory/src/main/scala/org/apache/samza/storage/kv/inmemory/InMemoryKeyValueStore.scala > 72f25a354eaa98e8df379d07d9cc8613dfafd13a > > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala > 211fc3be1e168f1f92812406785b39b5a3fd9174 > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueIterator.java > 854ebbfa640e96a87977ae25b67736ef57fadd8e > samza-kv/src/main/java/org/apache/samza/storage/kv/KeyValueStore.java > b1fea7bc38d1a44e7bba6bdeb638dc33c0936c6f > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStore.scala > 9a5b2d5f8f76220dfc8637823e2b63dffc138a5d > > samza-kv/src/main/scala/org/apache/samza/storage/kv/CachedStoreMetrics.scala > df8efaeefba829a47cb744d7a755cbe9e1f562f1 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala > e5a66a4770b9553a1cc48fbb505f52d123c6c754 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineMetrics.scala > 233fba91caf041bfb78189efef00ce8fc56f9f15 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStoreMetrics.scala > 967d5097253640ee41ee84e551e15fa2285c00e2 > samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStore.scala > 7bba6ff37d8266674e7f15c10c7c146f4a41fc91 > > samza-kv/src/main/scala/org/apache/samza/storage/kv/LoggedStoreMetrics.scala > 743151a310792d4a6ff48ea102e796eb9f630bba > > samza-kv/src/main/scala/org/apache/samza/storage/kv/NullSafeKeyValueStore.scala > 3de257c3117c16b7cfdb7a3d7bb28d5cddc7c10a > > samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala > 8e183efcdec6fd3f921fc2bfe1971c95715930ed > > samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStoreMetrics.scala > 841e4a236da99cbfe121054654c648887f39c3f6 > samza-kv/src/test/scala/org/apache/samza/storage/kv/MockKeyValueStore.scala > 595dd0df6fde50f91ab5a046a193559326f2a1d5 > > Diff: https://reviews.apache.org/r/41068/diff/ > > > Testing > ------- > > > Thanks, > > Amit Yadav > >
