Thanks for asking for clarification, Sophie; that gives me guidance on improving the KIP! Here's the updated version, including the JIRA link: https://cwiki.apache.org/confluence/display/KAFKA/KIP-941%3A+Range+queries+to+accept+null+lower+and+upper+bounds
On Thu, Jun 22, 2023 at 12:57 PM Sophie Blee-Goldman <ableegold...@gmail.com> wrote: > 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 > > > > > > -- [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>
