Re: [DISCUSS] KIP-763: Range queries with open endpoints
Boyang, Thanks for your comment. The concern about whether a null-based approach increases the chances of hidden bugs is a valid concern. In the end, the decision to go with this approach was guided by considering the effort and implications of changing the API towards a new range interface, versus the likelihood of such bugs happening. Changing an API is always a major effort. On other hand, I think the risk of hidden bugs with the null-based API are low, for two reasons. First, there should not be any existing application developed for the old API passing null, as null parameters did throw an exception in the old API. New applications passing null without the intention to do an open endpoint range query are, I would say, unlikely, and if they happen then it's clearly the application developer's mistake who did not read the API documentation. -Patrick On Sat, Aug 28, 2021 at 8:24 AM Boyang Chen wrote: > 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 > > 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 > 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 ot
Re: [DISCUSS] KIP-763: Range queries with open endpoints
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 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 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 > > > > > > > > >
Re: [DISCUSS] KIP-763: Range queries with open endpoints
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 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 > > > > >
Re: [DISCUSS] KIP-763: Range queries with open endpoints
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
Re: [DISCUSS] KIP-763: Range queries with open endpoints
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
[DISCUSS] KIP-763: Range queries with open endpoints
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