Hey Lucia, thanks for the KIP! Just some minor notes: I'm in favor of the proposal overall, at least I think so -- for someone not intimately familiar with the new IQ API and *RangeQuery* class, the KIP was a bit difficult to follow along and I had to read in between the lines to figure out what the old behavior was and what the new and improved logic would do.
It would be good to state clearly in the beginning what happens when null is passed in right now, and what will happen after this KIP is implemented. For example in the "Public Interfaces" section, I couldn't tell if the middle sentence was describing what was changing, or what it was changing *to.* One last little thing is can you link to the jira ticket at the top? And please create one if it doesn't already exist -- it helps people figure out when a KIP has been implemented and in which versions, as well as navigate from the KIP to the actual code that was merged. Things can change during implementation and the KIP document is how most people read up on new features, but almost all of us are probably guilty of forgetting to update the KIP document. So it's important to be able to find the code when in doubt. Otherwise nice KIP! On Thu, Jun 22, 2023 at 8:19 AM Lucia Cerchie <lcerc...@confluent.io.invalid> wrote: > 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 > > >
