Thanks everyone for voting. Voting passed with the following +1s:
- Luke Chen - Guozhang Wang (binding) - John Roesler (binding) - Bruno Cadonna (binding) I'll update the KIP status accordingly. Best, Patrick On Thu, Dec 16, 2021 at 2:09 PM Bruno Cadonna <cado...@apache.org> wrote: > Hi Partick, > > Thank you for the KIP! > > +1 (binding) > > Best, > Bruno > > On 16.12.21 07:22, Luke Chen wrote: > > Hi Patrick, John, > > > > Thanks for your explanation and update. > > It looks better now. > > > > +1 (non-binding) from me. > > > > just one minor comment: > > 10. In the example section, we are still using the variable names `lower` > > and `upper` as before. We should update them, too. (follow the decision > for > > point 7 above) > > > > Thank you. > > Luke > > > > On Thu, Dec 16, 2021 at 3:34 AM Guozhang Wang <wangg...@gmail.com> > wrote: > > > >> Thanks Patrick, John. > >> > >> I read through the updated KIP and it's much cleared on the scope now, > >> appreciate that! I'm +1 overall, modulo a few minor comments: > >> > >> 7. The parameter names of `WindowKeyQuery#withKeyAndWindowStartRange`, > i.e. > >> `startTime` and `endTime` seem not updated; also for the parameter > names of > >> `WindowRangeQuery#withWindowStartRange`, the `earliest / latest` seems > >> inconsistent with the existing API parameter names, how about naming > both > >> of them `fromWindowStartTime` and `toWindowStartTime`? > >> > >> 8. `getStartTime / getEndTime` are not updated as well: should they be > >> `getEarliestWindowStartTime` / `getLatestWindowStartTime` or `getFrom... > >> getTo...` if you agree with 7) above? > >> > >> 9. "One reason we cannot use WindowKeyQuery to support > >> SessionStore.fetch(key)": not sure I understand this part, for the new > APIs > >> we do not need to be following the old API's return types since they are > >> separated, right? So if we think it's actually better for the new API > >> mimicing `sessionStore.fetch(key)` to return a `WindowStoreIterator<V>`, > >> why can't we just do that? > >> > >> > >> Guozhang > >> > >> On Wed, Dec 15, 2021 at 8:49 AM John Roesler <vvcep...@apache.org> > wrote: > >> > >>> FYI, I filed this follow-on task to explore a more general > >>> pattern for these queries: > >>> https://issues.apache.org/jira/browse/KAFKA-13548 > >>> > >>> We can unblock the current scope for these queries but still > >>> plan to revisit the API before the first release of IQv2. > >>> > >>> Thanks! > >>> -John > >>> > >>> On Wed, 2021-12-15 at 10:34 -0600, John Roesler wrote: > >>>> Thanks for the update, Patrick! > >>>> > >>>> Tl;dr: I'm +1 (binding) > >>>> > >>>> I just reviewed the KIP again (I hope you don't mind, I > >>>> fixed a couple of missed renames in the text and examples). > >>>> > >>>> One of the design of IQv2 is to make proposing and evolving > >>>> these queries much less onerous. Unlike adding new methods > >>>> to all StateStore interfaces and implementations, if we want > >>>> to make (eg) the WindowRangeQuery more flexible in the > >>>> future, we can easily do so by just adding some builder > >>>> methods to set the bounds independently, or by adding new > >>>> static methods to provide different parameterizations. > >>>> > >>>> Therefore, even though this is not the ultimate expression > >>>> of what we think the range queries should be, it's a > >>>> perfectly fine starting point. The most pressing concerns to > >>>> me were the cases where Luke and Guozhang pointed out that > >>>> some parts of the proposal or interfaces were ambiguous, > >>>> which looks like it's fixed now. > >>>> > >>>> My preference would be to go ahead and accept this proposed > >>>> scope so that we have at least some basic key-value, window, > >>>> and session store queries in the code base. Until we have > >>>> those MVP queries in place, we can't really start to address > >>>> higher-level follow-up items like: > >>>> https://issues.apache.org/jira/browse/KAFKA-13541 (to refine > >>>> how the serdes interplay with the queries) or > >>>> https://issues.apache.org/jira/browse/KAFKA-13541 (to > >>>> consider alternative ways to pin down the generic type > >>>> parameters better). > >>>> > >>>> Anyway, for the current version of the KIP, my vote is +1 > >>>> (binding). > >>>> > >>>> Thanks again! > >>>> -John > >>>> > >>>> On Wed, 2021-12-15 at 12:53 +0100, Patrick Stuedi wrote: > >>>>> Thanks everyone for the sharing comments and suggestions, this is > >>>>> very helpful! > >>>>> > >>>>> I have updated the KIP based on the suggestions, please let me know > >>> what > >>>>> you think. Here are the main changes: > >>>>> - Removed constructor in WindowRangeQuery (Guozhang) > >>>>> - Clearly specified how each of the instantiation of the different > >>> query > >>>>> types maps to specific operations in WindowStore and SessionStore > >>> (Guozhang) > >>>>> - Renamed the window start end time parameters and getter methods > >>> (Luke) > >>>>> - Added some comments on the use of WindowRangeQuery.fromKey > >> (Guozhang, > >>>>> John) > >>>>> > >>>>> The KIP currently takes a minimalist approach to move things forward. > >>> There > >>>>> are two follow ups I can think of from this KIP: > >>>>> * Add additional parameter combinations to WindowRangeQuery, e.g., > >>> support > >>>>> for key range with and without window ranges. > >>>>> * Remove WindowRangeQuery.fromKey and either use > >>>>> WindowKeyQuery.withKeyAndWindowBounds or add a new call like > >>>>> WindowKeyQuery.withKeyAndNoWindowBounds(): either way that would then > >>> raise > >>>>> the question which operation in WindowStore this query should map to, > >>> given > >>>>> that WindowKeyQuery is templated against WindowStoreIterator and the > >>>>> current use of WindowRangeQuery.fromKey is to call SessionStore.fetch > >>> which > >>>>> returns a KeyValueIterator. > >>>>> * Combine WindowKeyQuery and WindowRangeQuery, e.g., by exposing the > >>>>> template type > >>>>> * Create a "Builder" interface for the query types to avoid blowing > >> up > >>> the > >>>>> static methods. > >>>>> > >>>>> Since I think any of those tasks will require some more discussion, > >>> and in > >>>>> the interest of being pragmatic here, I suggest we defer those > >>>>> optimizations to future KIPs. > >>>>> > >>>>> Best, > >>>>> Patrick > >>>>> > >>>>> On Tue, Dec 14, 2021 at 1:02 AM Guozhang Wang <wangg...@gmail.com> > >>> wrote: > >>>>> > >>>>>> Hi John, > >>>>>> > >>>>>> Please see my follow-up comments inlined below. > >>>>>> > >>>>>> On Mon, Dec 13, 2021 at 2:26 PM John Roesler <vvcep...@apache.org> > >>> wrote: > >>>>>> > >>>>>>> Hi Patrick, thanks for the KIP! > >>>>>>> > >>>>>>> I hope you, Guozhang, and Luke don't mind if I share some > >>>>>>> thoughts: > >>>>>>> > >>>>>>> 1. I think you just meant to remove that private constructor > >>>>>>> from the KIP, right? > >>>>>>> > >>>>>> > >>>>>> Yes. > >>>>>> > >>>>>> > >>>>>>> > >>>>>>> 2. I think WindowRangeQuery#withWindowRage(windowLower, > >>>>>>> windowUpper) is the version that yields an iterator over > >>>>>>> both keys and windows, so it's not totally redundant. > >>>>>>> > >>>>>>> However, it does seem like WindowRangeQuery#withKey is > >>>>>>> equivalent to WindowKeyQuery#withKey . I overlooked it > >>>>>>> before, but we probably don't need both. > >>>>>>> > >>>>>>> Stepping back, it seems like we could either just de- > >>>>>>> duplicate that specific withKey query or we could even just > >>>>>>> merge both use cases into the WindowRangeQuery. I've > >>>>>>> personally never been convinced that the extra complexity of > >>>>>>> having the WindowStoreIterator is worth the savings of not > >>>>>>> duplicating the key on each record. Maybe it would be if we > >>>>>>> separately deserialized the same key over and over again, > >>>>>>> but that seems optimizable. > >>>>>>> > >>>>>>> On the other hand, that's more work, and the focus for IQv2 > >>>>>>> right now is to just get some MVP of queries implemented to > >>>>>>> cover basic use cases, so it might be worthwhile to keep it > >>>>>>> simple (by just dropping `WindowRangeQuery#withKey`). I'm > >>>>>>> happy with whatever call Patrick wants to make here, since > >>>>>>> it's his work to do. > >>>>>>> > >>>>>> > >>>>>> My original comments was actually referring to that > >>> `WindowRangeQuery` does > >>>>>> not allow users to specify a range of [keyFrom: windowFrom, keyTo: > >>>>>> windowTo], but only [key: windowFrom, key: windowTo], or [-INF: > >>> windowFrom, > >>>>>> INF: windowTo] if we do not specify the key but only do > >>> `withWindowRage`. I > >>>>>> was not sure if this is intentional by design, i.e. that the only > >>>>>> difference between the two classes is that the latter allows [-INF: > >>>>>> windowFrom, INF: windowTo], and and I was assuming it's now, but > >>> otherwise > >>>>>> these two are then very much alike. > >>>>>> > >>>>>> So I guess my question is that, is it by-design to not allow users > >> do > >>>>>> something like `query.withWindowRange(..).withKeyRange(..)`? > >>>>>> > >>>>>> > >>>>>>> > >>>>>>> 3. I got the impression that WindowRangeQuery was intended > >>>>>>> for use with both Window and Session stores, but that might > >>>>>>> be my assumption. > >>>>>>> > >>>>>>> > >>>>>> Today the session store's query API is at least different on the > >>> names, > >>>>>> e.g. "findSessions", so I'm not sure if the proposal intend to > >>> replace > >>>>>> these functions with the WindowRangeQuery such that: > >>>>>> > >>>>>> sessionStore.findSessions(K, Ins, Ins) -> > >>> query.withKey().withWindowRange() > >>>>>> sessionStore.findSessions(K, K, Ins, Ins) -> > >>>>>> query.withKeyRange().withWindowRange() // again, assuming we > >> would > >>> want > >>>>>> `withKeyRange` as in 2) above. > >>>>>> sessionStore.fetch(K) -> query.withKey() > >>>>>> sessionStore.fetch(K, K) -> query.withKeyRange() > >>>>>> > >>>>>> Also, sessionStore.fetchSession(K, Ins, Ins) seems would not be > >>> supported > >>>>>> with the new API. I'm not enforcing that we have the complete > >>> coverage in > >>>>>> this KIP, and I'm with you that we would focus on getting some MVP > >>> out > >>>>>> sooner, but I'd like our KIP to clarify on what's really covered > >>> comparing > >>>>>> against the old API and what's not. > >>>>>> > >>>>>> > >>>>>> > >>>>>>> 4. That's good feedback. I think those names were inspired > >>>>>>> by the WindowStore methods that just say stuff like > >>>>>>> `timeFrom` and `timeTo`. The SessionStore does a better job > >>>>>>> of being unambiguous, since it says stuff like > >>>>>>> `earliestSessionEndTime` and `latestSessionStartTime`. > >>>>>>> > >>>>>>> And, actually, that brings up a point that I think we > >>>>>>> overlooked before. While the method signatures of the > >>>>>>> WindowStore and SessionStore methods look the same, the > >>>>>>> meanings of their arguments are not the same. In particular, > >>>>>>> the WindowStore ranges are always in reference to the window > >>>>>>> start time, while the SessionStore ranges may be in > >>>>>>> reference to the window start time or end time (or different > >>>>>>> combinations of both). > >>>>>>> > >>>>>>> I suspect that your instinct to cover both stores with the > >>>>>>> same Query is correct, and we just need to be specific about > >>>>>>> the kinds of ranges we're talking about. For example, > >>>>>>> instead of withWindowRange, maybe we would have: > >>>>>>> > >>>>>>> withWindowStartRange( > >>>>>>> Instant earliestWindowStartTime, > >>>>>>> Instant latestWindowStartTime > >>>>>>> ); > >>>>>>> > >>>>>>> Then in the future, we could add stuff like: > >>>>>>> > >>>>>>> withWindowStartAndEndRange( > >>>>>>> Instant earliestWindowStartTime, > >>>>>>> Instant latestWindowEndTime > >>>>>>> ); > >>>>>>> > >>>>>>> Etc. > >>>>>>> > >>>>>>> 5. I think Luke's question reveals how weirdly similar but > >>>>>>> not the same these two queries are. It's not your fault, > >>>>>>> this is inherited from the existing set of weirdly-similar- > >>>>>>> but-not-the-same store methods. The thing that distinguishes > >>>>>>> the WindowKeyQuery from the WindowRangeQuery is: > >>>>>>> * WindowKeyQuery: all results share the same K key (cf point > >>>>>>> 2: I don't think we need `WindowRangeQuery#withKey(k)`). > >>>>>>> Therefore, the iterator is just (windowStartTime, value) > >>>>>>> pairs. > >>>>>>> * WindowRangeQuery: the results may have different keys, so > >>>>>>> the iterator is (Windowed<K>, value) pairs, where > >>>>>>> Windowed<K> is basically a (windowStartTime, K key) pair. > >>>>>>> > >>>>>>> Therefore, we could support a WindowRangeQuery with a fixed > >>>>>>> key, but it would just be another way to express the same > >>>>>>> thing as the WindowKeyQuery with the same parameters. > >>>>>>> > >>>>>>> > >>>>>> As I suggested, we do not necessarily need to cover all existing > >>> cases > >>>>>> compared with the old API, but it's better to be very clear on > >> what's > >>>>>> actually covered v.s. what's not. For example in WindowRangeQuery, > >>> I'd like > >>>>>> to clarify if all the following are intended in this KIP's scope: > >>>>>> > >>>>>> * query.withKey().withWindowRange() > >>>>>> * query.withKey() // range all window > >>>>>> * query.withWindowRange() // range all keys. > >>>>>> > >>>>>> > >>>>>>> As in point 2, I do think it might be worth converging all > >>>>>>> use cases into the WindowRangeQuery, but we can also just > >>>>>>> shoot for parity now and defer elegance for the future. > >>>>>>> > >>>>>>> 6. I'll just add one question of my own here: If we do keep > >>>>>>> both query classes, then I think we would drop `withKey` and > >>>>>>> `getKey` from the WindowRangeQuery. But if we do wind up > >>>>>>> keeping the WindowRangeQuery class, I think we should > >>>>>>> reconsider the name of the getter, since there are other > >>>>>>> range queries that support a range of keys. Is there a > >>>>>>> getter name we can use that would still work if we come back > >>>>>>> later and add lower and upper key bounds? > >>>>>>> > >>>>>>> Thanks again for the KIP! > >>>>>>> John > >>>>>>> > >>>>>>> On Mon, 2021-12-13 at 16:35 +0800, Luke Chen wrote: > >>>>>>>> Hi Patrick, > >>>>>>>> > >>>>>>>> Thanks for the KIP! > >>>>>>>> > >>>>>>>> I have some comments, in addition to Guozhang's comments: > >>>>>>>> 4. The parameter names `windowLower` and `windowUpper` are kind > >>> of > >>>>>>>> ambiguous to me. Could we come up a better name for it, like > >>>>>>>> `windowStartTime`, `windowEndTime`, or even we don't need the > >>> "window" > >>>>>>>> name, just `startTime` and `endTime`? > >>>>>>>> 5. Why can't we support window range query with a key within a > >>> time > >>>>>>> range? > >>>>>>>> You might need to explain in the KIP. > >>>>>>>> > >>>>>>>> Thank you. > >>>>>>>> Luke > >>>>>>>> > >>>>>>>> > >>>>>>>> On Sat, Dec 11, 2021 at 7:54 AM Guozhang Wang < > >>> wangg...@gmail.com> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Hi Patrick, > >>>>>>>>> > >>>>>>>>> I made a pass on the KIP and have a few comments below: > >>>>>>>>> > >>>>>>>>> 1. The `WindowRangeQuery` has a private constructor while the > >>>>>>>>> `WindowKeyQuery` has not, is that intentional? > >>>>>>>>> > >>>>>>>>> 2. The `WindowRangeQuery` seems not allowing to range over > >> both > >>>>>> window > >>>>>>> and > >>>>>>>>> key, but only window with a fixed key, in that case it seems > >>> pretty > >>>>>>> much > >>>>>>>>> the same as the other (ignoring the constructor), since we > >>> know we > >>>>>>> would > >>>>>>>>> only have a single `key` value in the returned iterator, and > >>> hence it > >>>>>>> seems > >>>>>>>>> returning in the form of `WindowStoreIterator<V>` is also > >> fine > >>> as the > >>>>>>> key > >>>>>>>>> is fixed and hence no need to maintain it in the returned > >>> iterator. > >>>>>> I'm > >>>>>>>>> wondering should we actually support ranging over keys as > >> well > >>> in > >>>>>>>>> `WindowRangeQuery`? > >>>>>>>>> > >>>>>>>>> 3. The KIP title mentioned both session and window, but the > >>> APIs only > >>>>>>>>> involves window stores; However the return type > >>> `WindowStoreIterator` > >>>>>>> is > >>>>>>>>> only for window stores not session stores, so I feel we would > >>> still > >>>>>>> have > >>>>>>>>> some differences for session window query interface? > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Guozhang > >>>>>>>>> > >>>>>>>>> On Fri, Dec 10, 2021 at 1:32 PM Patrick Stuedi > >>>>>>>>> <pstu...@confluent.io.invalid> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hi everyone, > >>>>>>>>>> > >>>>>>>>>> I would like to start the vote for KIP-806 that adds window > >>> and > >>>>>>> session > >>>>>>>>>> query support to query KV-stores using IQv2. > >>>>>>>>>> > >>>>>>>>>> The KIP can be found here: > >>>>>>>>>> https://cwiki.apache.org/confluence/x/LJaqCw > >>>>>>>>>> > >>>>>>>>>> Skipping the discussion phase as this KIP is following the > >>> same > >>>>>>> pattern > >>>>>>>>> as > >>>>>>>>>> the previously submitted KIP-805 (KIP: > >>>>>>>>>> https://cwiki.apache.org/confluence/x/85OqCw, Discussion: > >>>>>>>>>> https://tinyurl.com/msp5mcb2). Of course concerns/comments > >>> can > >>>>>>> still be > >>>>>>>>>> brought up in this thread. > >>>>>>>>>> > >>>>>>>>>> -Patrick > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> -- Guozhang > >>>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> -- > >>>>>> -- Guozhang > >>>>>> > >>>> > >>> > >>> > >> > >> -- > >> -- Guozhang > >> > > >
