Thank you Richard! Do you or Guozhang have any thoughts on my suggestions to use fetchAll() and fetchAll(timeFrom, timeTo) and reserve the "range" keyword for when we query a specific range of keys?
Xavier On Sat, Oct 14, 2017 at 2:32 PM Richard Yu <yohan.richard...@gmail.com> wrote: > Thanks for the clarifications, Xavier. > I have removed most of the methods except for keys() and all() which has > been renamed to Guozhang Wang's suggestions. > > Hope this helps. > > On Fri, Oct 13, 2017 at 3:28 PM, Xavier Léauté <xav...@confluent.io> > wrote: > > > Thanks for the KIP Richard, this is a very useful addition! > > > > As far as the API changes, I just have a few comments on the methods that > > don't seem directly related to the KIP title, and naming of course :). > > On the implementation, see my notes further down that will hopefully > > clarify a few things. > > > > Regarding the "bonus" methods: > > I agree with Guozhang that the KIP lacks proper motivation for adding the > > min, max, and allLatest methods. > > It is also not clear to me what min and max would really mean, what > > ordering do we refer to here? Are we first ordering by time, then key, or > > first by key, then time? > > The allLatest method might be useful, but I don't really see how it would > > be used in practice if we have to scan the entire range of keys for all > the > > state stores, every single time. > > > > Maybe we could flesh the motivation behind those extra methods, but in > the > > interest of time, and moving the KIP forward it might make sense to file > a > > follow-up once we have more concrete use-cases. > > > > On naming: > > I also agree with Guozhang that "keys()" should be renamed. It feels a > bit > > of a misnomer, since it not only returns keys, but also the values. > > > > As far as what to rename it to, I would argue we already have some > > discrepancy between key-value stores using range() vs. window stores > using > > fetch(). > > I assume we called the window method "fetch" instead of "get" because you > > might get back more than one window for the requested key. > > > > If we wanted to make things consistent with both existing key-value store > > naming and window store naming, we could do the following: > > Decide that "all" always refers to the entire range of keys, independent > of > > the window and similarly "range" always refers to a particular range of > > keys, irrespective of the window. > > We can then prefix methods with "fetch" to indicate that more than one > > window may be returned for each key in the range. > > > > This would give us: > > - a new fetchAll() method for all the keys, which makes it clear that you > > might get back the same key in different windows > > - a new fetchAll(timeFrom, timeTo) method to get all the keys in a given > > time range, again with possibly more than one window per key > > - and we'd have to rename fetch(K,K,long, long) to fetchRange(K, K, long, > > long) and deprecate the old one to indicate a range of keys > > > > One inconsistency I noted: the "Proposed Changes" section in your KIP > talks > > about a "range(timeFrom, timeTo)" method, I think you meant to refer to > the > > all(from, to) method, but I'm sure you'll fix that once we decide on > > naming. > > > > On the implementation side: > > You mentioned that caching and rocksdb store have very different > key/value > > structures, and while it appears to be that way on the surface, the > > structure between the two is actually very similar. Keys in the cache are > > prefixed with a segment ID to ensure the ordering in the cache stays > > consistent with the rocksdb implementation, which maintains multiple > > rocksdb instances, one for each segment. So we just "artificially" mirror > > the segment structure in the cache. > > > > The reason for keeping the ordering consistent is pretty simple: keep in > > mind that when we query a cached window store we are effectively querying > > both the cache and the persistent rocksdb store at the same time, merging > > results from both. To make that merge as painless as possible, we ensure > > the ordering is consistent when querying a range of keys in both stores. > > > > Also keep in mind CompositeReadonlyWindowStore, which wraps multiple > window > > stores within a topology. > > > > Hope this clarifies some of the less trivial parts of caching window > store. > > > > Cheers, > > Xavier > > > > On Sun, Oct 8, 2017 at 9:21 PM Guozhang Wang <wangg...@gmail.com> wrote: > > > > > Richard, Matthias: > > > > > > 0. Could you describe a bit what are the possible use cases of > > `allLatest`, > > > `minKey` and `maxKey`? I'd prefer keeping the APIs to add at a minimum > > > necessary amount, to avoid a swamp of new APIs that no one would really > > use > > > but just complicated the internal code base. > > > > > > 1. One minor comment on the other two new APIs: could we rename `keys` > to > > > `all` and `all` to `range` to be consistent with the other store's > APIs? > > > > > > 2. One meta comment on the implementation details: since both `keys` > and > > > `all` would likely touch multiple segments, we may need to use the > > internal > > > `SegmentIterator` class, but currently it always requires a Bytes from > > and > > > Bytes to for its constructor. What changes we need to make for that > > class? > > > > > > > > > Guozhang > > > > > > > > > On Thu, Oct 5, 2017 at 4:42 PM, Richard Yu <yohan.richard...@gmail.com > > > > > wrote: > > > > > > > We should split KAFKA-4499 into several sub-issues with 4499 being > the > > > > parent issue. > > > > Adding the implementation to CachingWindowStore, RocksDBWindowStore, > > etc > > > > will each require the addition of a test and implementing the methods > > > which > > > > is not trivial. > > > > This way, it should be easier to manage the progress of the KIP. > > > > > > > > > > > > On Thu, Oct 5, 2017 at 2:58 PM, Matthias J. Sax < > matth...@confluent.io > > > > > > > wrote: > > > > > > > > > Thanks for driving this and sorry for late response. With release > > > > > deadline it was pretty busy lately. > > > > > > > > > > Can you please add a description for the suggested method, what > they > > > are > > > > > going to return? It's a little unclear to me atm. > > > > > > > > > > It would also be helpful to discuss, for which use case each method > > is > > > > > useful. This might also help to identify potential gaps for which > > > > > another API might be more helpful. > > > > > > > > > > Also, we should talk about provided guarantees when using those > APIs > > > > > with regard to consistency -- not saying that we need to provide > > strong > > > > > guarantees, but he KIP should describe what user can expect. > > > > > > > > > > > > > > > -Matthias > > > > > > > > > > On 9/24/17 8:11 PM, Richard Yu wrote: > > > > > > Hello, I would like to solicit review and comment on this issue > > (link > > > > > > below): > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > 205%3A+Add+getAllKeys%28%29+API+to+ReadOnlyWindowStore > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > -- Guozhang > > > > > >
