Thanks Michal, you are correct. I can see your point now, and I can get behind returning Windowed<K> as well for windowed stores.
It might make sense to revisit the single key iterator in the future and do the same for consistency, but I'd rather not break backwards compatibility unless we have a good reason to do so. Everyone else who already voted on the KIP, are there any objections to this change? I have updated the KIP accordingly. Thank you, Xavier On Thu, May 11, 2017 at 12:44 AM Michal Borowiecki < michal.borowie...@openbet.com> wrote: > Also, wrt > > > In the case of the window store, the "key" of the single-key iterator is > > the actual timestamp of the underlying entry, not just range of the > > window, > > so if we were to wrap the result key a window we wouldn't be getting back > > the equivalent of the single key iterator. > I believe the timestamp in the entry *is* the window start time (the end > time is implicitly known by adding the window size to the window start > time) > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamWindowReduce.java#L109 > > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamWindowAggregate.java#L111 > > Both use window.start() as the timestamp when storing into the WindowStore. > > Or am I confusing something horribly here? Hope not ;-) > > > If the above is correct, then using KeyValueIterator<Windowed<K>, V> as > the return type of the new fetch method would indeed not lose anything > the single key iterator is offering. > > The window end time could simply be calculated as window start time + > window size (window size would have to be passed from the store supplier > to the store implementation, which I think it isn't now but that's an > implementation detail). > > If you take objection to exposing the window end time because the single > key iterator doesn't do that, then an alternative could also be to have > the return type of the new fetch be something like > KeyValueItarator<Tuple2<K, Long>, V>, since the key is composed of the > actual key and the timestamp together. peakNextKey() would then allow > you to glimpse both the actual key and the associated window start time. > This feels like a better workaround then putting the KeyValue pair in > the V of the WindowStoreIterator<V>. > > All-in-all, I'd still prefer KeyValueIterator<Windowed<K>, V> as it more > clearly names what's what. > > What do you think? > > Thanks, > > Michal > > On 11/05/17 07:51, Michal Borowiecki wrote: > > Well, another concern, apart from potential confusion, is that you > > won't be able to peek the actual next key, just the timestamp. So the > > tradeoff is between having consistency in return types versus > > consistency in having the ability to know the next key without moving > > the iterator. To me the latter just feels more important. > > > > Thanks, > > Michal > > On 11 May 2017 12:46 a.m., Xavier Léauté <xav...@confluent.io> wrote: > > > > Thank you for the feedback Michal. > > > > While I agree the return may be a little bit more confusing to reason > > about, the reason for doing so was to keep the range query interfaces > > consistent with their single-key counterparts. > > > > In the case of the window store, the "key" of the single-key > > iterator is > > the actual timestamp of the underlying entry, not just range of > > the window, > > so if we were to wrap the result key a window we wouldn't be > > getting back > > the equivalent of the single key iterator. > > > > In both cases peekNextKey is just returning the timestamp of the > > next entry > > in the window store that matches the query. > > > > In the case of the session store, we already return Windowed<K> > > for the > > single-key method, so it made sense there to also return > > Windowed<K> for > > the range method. > > > > Hope this make sense? Let me know if you still have concerns about > > this. > > > > Thank you, > > Xavier > > > > On Wed, May 10, 2017 at 12:25 PM Michal Borowiecki < > > michal.borowie...@openbet.com> wrote: > > > > > Apologies, I missed the discussion (or lack thereof) about the > > return > > > type of: > > > > > > WindowStoreIterator<KeyValue<K, V>> fetch(K from, K to, long > > timeFrom, > > > long timeTo) > > > > > > > > > WindowStoreIterator<V> (as the KIP mentions) is a subclass of > > > KeyValueIterator<Long, V> > > > > > > KeyValueIterator<K,V> has the following method: > > > > > > /** * Peek at the next key without advancing the iterator * > > @return the > > > key of the next value that would be returned from the next call > > to next > > > */ K peekNextKey(); > > > > > > Given the type in this case will be Long, I assume what it would > > return > > > is the window timestamp of the next found record? > > > > > > > > > In the case of WindowStoreIterator<V> fetch(K key, long > > timeFrom, long > > > timeTo); > > > all records found by fetch have the same key, so it's harmless > > to return > > > the timestamp of the next found window but here we have varying > > keys and > > > varying windows, so won't it be too confusing? > > > > > > KeyValueIterator<Windowed<K>, V> (as in the proposed > > > ReadOnlySessionStore.fetch) just feels much more intuitive. > > > > > > Apologies again for jumping onto this only once the voting has > > already > > > begun. > > > Thanks, > > > Michał > > > > > > On 10/05/17 20:08, Sriram Subramanian wrote: > > > > +1 > > > > > > > > On Wed, May 10, 2017 at 11:42 AM, Bill Bejeck > > <bbej...@gmail.com> wrote: > > > > > > > >> +1 > > > >> > > > >> Thanks, > > > >> Bill > > > >> > > > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang > > <wangg...@gmail.com> > > > wrote: > > > >> > > > >>> +1. Thank you! > > > >>> > > > >>> On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > > <xav...@confluent.io> > > > >>> wrote: > > > >>> > > > >>>> Hi everyone, > > > >>>> > > > >>>> Since there aren't any objections to this addition, I would > > like to > > > >> start > > > >>>> the voting on KIP-155 so we can hopefully get this into 0.11. > > > >>>> > > > >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP+ > > > >>>> 155+-+Add+range+scan+for+windowed+state+stores > > > >>>> > > > >>>> Voting will stay active for at least 72 hours. > > > >>>> > > > >>>> Thank you, > > > >>>> Xavier > > > >>>> > > > >>> > > > >>> > > > >>> -- > > > >>> -- Guozhang > > > >>> > > > > > > > > > >
