Thanks Kirk and John for the valuable feedback! John, I'll update the KIP to reflect that nuance you mention -- yes it is just about making the withRange method more permissive. Thanks for the testing file as well, I'll be sure to write my test cases there.
On Wed, Jun 21, 2023 at 10:50 AM Kirk True <k...@kirktrue.pro> wrote: > 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 > > > >>> > > -- [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>
