Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
Subject: Re: [VOTE] KIP-155 Add range scan for windowed state stores
To: dev@kafka.apache.org
References: <CA+rPSbZs9yLkUsXkrR1w3_oejf4d1D6SE6hkV50s4fNZKM4s8g@mail.gmail.com>
 <CAHwHRrUbocp9KTV9xgYmZMRwm-0hC-c4f5JfSVADJDTw0ZB0cQ@mail.gmail.com>
 <CAF7WS+oOGDMapRCuSEu7DvHRABMnY67wZZbRGx+mj41DLuumuQ@mail.gmail.com>
 <CAG+EsUdvLUQRWBha49c9KBbzOJq_GUDp8hKSODakpH4jtorAPQ@mail.gmail.com>
 <af7a5e28-8de6-9934-f575-3a85cc0f4b07@openbet.com>
 <CA+rPSbbe5aKhQPtTXnUL3ZBTAj1_eqnjyDiVOgVW2TnNQocoeQ@mail.gmail.com>
 <481d3b87-bcc3-472c-ac3a-fa44603735d7@openbet.com>
From: Michal Borowiecki <michal.borowiecki@openbet.com>
Message-ID: <058bad63-8d10-e1ce-541d-3b98be35c53f@openbet.com>
Date: Thu, 11 May 2017 08:44:21 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <481d3b87-bcc3-472c-ac3a-fa44603735d7@openbet.com>
Content-Type: multipart/alternative;
 boundary="------------E4A01C20B4AC898E21C45D88"
archived-at: Thu, 11 May 2017 07:44:33 -0000

--------------E4A01C20B4AC898E21C45D88
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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é <xavier@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.borowiecki@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
>     <bbejeck@gmail.com> wrote:
>     > >
>     > >> +1
>     > >>
>     > >> Thanks,
>     > >> Bill
>     > >>
>     > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang
>     <wangguoz@gmail.com>
>     > wrote:
>     > >>
>     > >>> +1. Thank you!
>     > >>>
>     > >>> On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté
>     <xavier@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
>     > >>>
>     >
>     >
>


--------------E4A01C20B4AC898E21C45D88--