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
