Hi John/Lucia, Thanks for the feedback!
Of course I only noticed the private-ness of the RangeQuery constructor moments after sending my email ¯\_(ツ)_/¯ Just to be clear, I’m happy with the proposed change as it conforms to Postel’s Law ;) Apologies that it was worded tersely. Thanks, Kirk > On Jun 21, 2023, at 10:20 AM, John Roesler <vvcep...@apache.org> wrote: > > Hi all, > > Thanks for the KIP, Lucia! This is a nice change. > > To Kirk's question (1), the example is a bit misleading. The typical case > that would ease user pain is specifically using "null" to indicate an > open-ended range, especially since null is not a valid key. > > I could additionally see an empty string as being nice, but the actual API is > generic, not String, so there's no meaningful concept of > empty/blank/whitespace that we could check for, just null or not. > > Regarding (2), there's no public factory that takes Optional parameters. I > think you're looking at the private constructor. An alternative Lucia could > consider is to instead propose adding a new factory like > `withRange(Optional<K> lower, Optional<K> upper)`. > > FWIW, I'd be in favor of this KIP as proposed. > > A couple of smaller notes: > > 3. In the compatibility notes, I wasn't sure what "web request" was referring > to. I think you just mean that all existing valid API calls will continue to > work the same, and we're only making the withRange method more permissive > with its arguments. > > 4. For the Test Plan, I wrote some tests that validate these queries against > every kind and configuration of store possible. Please add your new test > cases to that one to make absolutely sure it'll work for every store. > Obviously, you may also want to add some specific unit tests in addition. > > See > https://github.com/apache/kafka/blob/trunk/streams/src/test/java/org/apache/kafka/streams/integration/IQv2StoreIntegrationTest.java > > Thanks again! > -John > > On 6/21/23 12:00, Kirk True wrote: >> Hi Lucia, >> One question: >> 1. Since the proposed implementation change for withRange() method uses >> Optional.ofNullable() (which only catches nulls and not blank/whitespace >> strings), wouldn’t users still need to have code like that in the example? >> 2. Why don't users create RangeQuery objects that use Optional directly? >> What’s the benefit of introducing what appears to be a very thin utility >> facade? >> Thanks, >> Kirk >>> On Jun 21, 2023, at 9:51 AM, Kirk True <k...@kirktrue.pro> wrote: >>> >>> Hi Lucia, >>> >>> Thanks for the KIP! >>> >>> The KIP wasn’t in the email and I didn’t see it on the main KIP directory. >>> Here it is: >>> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-941%3A+Range+queries+to+accept+null+lower+and+upper+bounds >>> >>> Can the KIP be added to the main KIP page >>> (https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals)? >>> That will help with discoverability and encourage discussion. >>> >>> Thanks, >>> Kirk >>> >>>> On Jun 15, 2023, at 2:13 PM, Lucia Cerchie <lcerc...@confluent.io.INVALID> >>>> wrote: >>>> >>>> Hi everyone, >>>> >>>> I'd like to discuss KIP-941, which will change the behavior of range >>>> queries to make it easier for users to execute full range scans when using >>>> interactive queries with upper and lower bounds from query parameters in >>>> web client requests. >>>> >>>> I much appreciate your input! >>>> >>>> Lucia Cerchie >>>> -- >>>> >>>> [image: Confluent] <https://www.confluent.io> >>>> Lucia Cerchie >>>> Developer Advocate >>>> Follow us: [image: Blog] >>>> <https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog>[image: >>>> Twitter] <https://twitter.com/ConfluentInc>[image: Slack] >>>> <https://slackpass.io/confluentcommunity>[image: YouTube] >>>> <https://youtube.com/confluent> >>>> >>>> [image: Try Confluent Cloud for Free] >>>> <https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic> >>>
