Thanks Patrick for the KIP and sorry for being late to the party here. One thing I would like to understand is, what's the expected behavior when a user specifies a null key in previous versions? In the new version which uses null key for open-ended range queries, could it potentially hide bugs in the user's code when their intention was to do a bounded range query but accidentally set the key to null?
Boyang On Wed, Jul 21, 2021 at 2:59 AM Patrick Stuedi <[email protected]> wrote: > Thanks John, Bruno for the valuable feedback! > > John: I had a quick look at the SessionStore and WindowStore interface. > While it looks like we should be able to apply similar ideas to those > stores, the actual interfaces are slightly different and expanding them for > open ranges may need a bit more thinking. In that sense, and to make sure > we will be converging with this KIP, I'd prefer to not expand the scope of > the KIP . As Bruno suggested we can always propose changes to the > SessionStore and WindowStore in a separate KIP. I'll add a subsection in > the rejected alternatives. > > @Bruno: good point, I'll add an example. Yes, all() will be equivalent to > range(null, null) and the implementation of all() will be a call to > range(null, null). > > -Patrick > > On Wed, Jul 21, 2021 at 9:12 AM Bruno Cadonna <[email protected]> wrote: > > > Thanks for the KIP, Patrick! > > > > I agree with John that your KIP is well motivated. > > > > I have just one minor feedback. Could you add store.range(null, null) to > > the example snippets with the comment that this is equivalent to all() > > (it is equivalent, right?)? This question about equivalence between > > range(null, null) and all() came up in my mind when I read the KIP and I > > think I am not the only one. > > > > Regarding expanding the KIP to SessionStore and WindowStore, I think you > > should not expand scope of the KIP. We can do the changes to the > > SessionStore and WindowStore in a separate KIP. But it is your call! > > > > > > Best, > > Bruno > > > > On 21.07.21 00:18, John Roesler wrote: > > > Thanks for the KIP, Patrick! > > > > > > I think your KIP is well motivated. It's definitely a bummer > > > to have to iterate over the full store as a workaround for > > > open-ended ranges. > > > > > > I agree with your pragmatic approach here. We have recently > > > had a couple of other contributions to the store APIs > > > (prefix and reverseRange), and the complexity of adding > > > those new methods far exceeded what anyone expected. I'd be > > > in favor of being conservative with new store APIs until we > > > are able to reign in that complexity somehow. Since your > > > proposed semantics seem reasonable, I'm in favor. > > > > > > While reviewing your KIP, it struck me that we have several > > > range-like APIs on: > > > * SessionStore > > > ( > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/state/ReadOnlySessionStore.java > > ) > > > * WindowStore > > > ( > > > https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/state/ReadOnlyWindowStore.java > > ) > > > as well. > > > > > > Do you think it would be a good idea to also propose a > > > similar change on those APIs? It might be nice (for exactly > > > the same reasons you documented), but it also increases the > > > scope of your work. There is one extra wrinkle I can see: > > > SessionStore has versions of the range methods that take a > > > `long` instead of an `Instant`, so `null` wouldn't work for > > > them. > > > > > > If you prefer to keep your KIP narrow in scope to just the > > > KeyValueStore, I'd also support you. In that case, it might > > > be a good idea to simply mention in the "Rejected > > > Alternatives" that you decided not to address those other > > > store APIs at this time. That way, people later on won't > > > have to wonder why those other methods didn't get updated. > > > > > > Other than that, I have no concerns! > > > > > > Thank you, > > > John > > > > > > On Mon, 2021-07-19 at 13:22 +0200, Patrick Stuedi wrote: > > >> Hi everyone, > > >> > > >> I would like to start the discussion for KIP-763: Range queries with > > open > > >> endpoints. > > >> > > >> The KIP can be found here: > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-763%3A+Range+queries+with+open+endpoints > > >> > > >> Any feedback will be highly appreciated. > > >> > > >> Many thanks, > > >> Patrick > > > > > > > > >
