Re: [VOTE] KIP-155 Add range scan for windowed state stores
Hi, Given the lack of any further comments or new votes, I'm assuming everyone who already voted is still in agreement and I will close the vote. The vote passed with 3 binding +1 votes (Guozhang, Sriram, Jason) and 2 non-binding +1 votes (Bill, Eno) Thank you, Xavier On Fri, May 12, 2017 at 9:22 AM Guozhang Wang wrote: > Thanks for the note Michael, I think it makes sense. > > And thinking about it more, I also agree that even for single-key fetch on > ReadOnlyWindow/SessionStore it's better to return the Iterator, > V> even when the windowed key's key field would always be the same, as it > is more consistent with the added APIs. But we need to do it in a backward > compatible way, which we can discuss in a follow-up KIP. > > > Guozhang > > On Thu, May 11, 2017 at 10:38 AM, Xavier Léauté > wrote: > > > Thanks Michal, you are correct. I can see your point now, and I can get > > behind returning Windowed 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, 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, 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. > > > > > > All-in-all, I'd still prefer KeyValueIterator, 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é > 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. > > > > > > > >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
Thanks for the note Michael, I think it makes sense. And thinking about it more, I also agree that even for single-key fetch on ReadOnlyWindow/SessionStore it's better to return the Iterator, V> even when the windowed key's key field would always be the same, as it is more consistent with the added APIs. But we need to do it in a backward compatible way, which we can discuss in a follow-up KIP. Guozhang On Thu, May 11, 2017 at 10:38 AM, Xavier Léauté wrote: > Thanks Michal, you are correct. I can see your point now, and I can get > behind returning Windowed 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, 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, 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. > > > > All-in-all, I'd still prefer KeyValueIterator, 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é 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 > > > for the > > > single-key method, so it made sense there to also return > > > Windowed 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: >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
Thanks Michal, you are correct. I can see your point now, and I can get behind returning Windowed 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, 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, 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. > > All-in-all, I'd still prefer KeyValueIterator, 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é 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 > > for the > > single-key method, so it made sense there to also return > > Windowed 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> fetch(K from, K to, long > > timeFrom, > > > long timeTo) > > > > > > > > > WindowStoreIterator (as the KIP mentions) is a subclass of > > > KeyValueIterator > > > > > > KeyValueIterator 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 W
Re: [VOTE] KIP-155 Add range scan for windowed state stores
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, 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, 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. All-in-all, I'd still prefer KeyValueIterator, 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é 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 for the single-key method, so it made sense there to also return Windowed 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> fetch(K from, K to, long timeFrom, > long timeTo) > > > WindowStoreIterator (as the KIP mentions) is a subclass of > KeyValueIterator > > KeyValueIterator 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 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, 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 wrote: > > > >> +1 > >> > >> Thanks, > >> Bill > >> > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang > wrote: > >> > >>> +1. Thank you! > >>> > >>> On Wed, M
Re: [VOTE] KIP-155 Add range scan for windowed state stores
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é 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 for the single-key method, so it made sense there to also return Windowed 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> fetch(K from, K to, long timeFrom, > long timeTo) > > > WindowStoreIterator (as the KIP mentions) is a subclass of > KeyValueIterator > > KeyValueIterator 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 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, 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 wrote: > > > >> +1 > >> > >> Thanks, > >> Bill > >> > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang > wrote: > >> > >>> +1. Thank you! > >>> > >>> On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > >>> 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 > >>> > >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
+1 Thanks Eno > On 10 May 2017, at 20:25, Michal Borowiecki > wrote: > > Apologies, I missed the discussion (or lack thereof) about the return type of: > > WindowStoreIterator> fetch(K from, K to, long timeFrom, long > timeTo) > > > WindowStoreIterator (as the KIP mentions) is a subclass of > KeyValueIterator > > KeyValueIterator 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 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, 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 wrote: >> >>> +1 >>> >>> Thanks, >>> Bill >>> >>> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang wrote: >>> +1. Thank you! On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté 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 >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
+1 On Wed, May 10, 2017 at 1:45 PM, Xavier Léauté 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 for the > single-key method, so it made sense there to also return Windowed 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> fetch(K from, K to, long timeFrom, > > long timeTo) > > > > > > WindowStoreIterator (as the KIP mentions) is a subclass of > > KeyValueIterator > > > > KeyValueIterator 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 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, 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 > wrote: > > > > > >> +1 > > >> > > >> Thanks, > > >> Bill > > >> > > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang > > wrote: > > >> > > >>> +1. Thank you! > > >>> > > >>> On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > > > >>> 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 > > >>> > > > > >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
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 for the single-key method, so it made sense there to also return Windowed 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> fetch(K from, K to, long timeFrom, > long timeTo) > > > WindowStoreIterator (as the KIP mentions) is a subclass of > KeyValueIterator > > KeyValueIterator 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 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, 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 wrote: > > > >> +1 > >> > >> Thanks, > >> Bill > >> > >> On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang > wrote: > >> > >>> +1. Thank you! > >>> > >>> On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > >>> 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 > >>> > >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
Apologies, I missed the discussion (or lack thereof) about the return type of: WindowStoreIterator> fetch(K from, K to, long timeFrom, long timeTo) WindowStoreIterator (as the KIP mentions) is a subclass of KeyValueIterator KeyValueIterator 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 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, 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 wrote: +1 Thanks, Bill On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang wrote: +1. Thank you! On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté 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
Re: [VOTE] KIP-155 Add range scan for windowed state stores
+1 On Wed, May 10, 2017 at 11:42 AM, Bill Bejeck wrote: > +1 > > Thanks, > Bill > > On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang wrote: > > > +1. Thank you! > > > > On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > > 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 > > >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
+1 Thanks, Bill On Wed, May 10, 2017 at 2:38 PM, Guozhang Wang wrote: > +1. Thank you! > > On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté > 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 >
Re: [VOTE] KIP-155 Add range scan for windowed state stores
+1. Thank you! On Wed, May 10, 2017 at 11:30 AM, Xavier Léauté 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
