Thanks for updating the KIP. Just re-read it. Overall LGTM.
A few nits:
single-key lookup with timestamp (upper) bound
Not sure if "(upper) bound" is the right phrasing? Personally, I find it
a little bit confusing, because the term "bound" kinda indicates a
range, but we still do a point
Thanks for the KIP Alieh!
I don't have anything to add to the 960 discussion right now as it seems
rather straightforward. I think after you address Bruno's comments we can
bring it to a vote. I'll review the two spawned KIPs separately.
Keep it up,
Walker
On Wed, Sep 6, 2023 at 5:11 AM Bruno
Hi Alieh,
I am sorry if I might repeat things that have been already said since I
am not sure I got all e-mails of this discussion thread.
The KIP looks good!
I just have two minor comments that I think are easily resolved.
1.
Why is defining latest() not needed? Is it because if I do not
Thank you, Matthias and Victoria.
Regarding the problem of using methods of single-key-single-ts queries for
KeyQuery (such as asOf) and vice versa (such as skipCache()), after a
discussion, we decided to define a separate class for single-key-single-ts
queries named VersionedKeyQuery.
Hi Alieh,
Thanks for the updates!
Some questions on the new limited-scope KIP:
1. The example in the "Examples" section shows the query type as
`KeyQuery>` and the result type as
`StateQueryResult>`. Should those have
`VersionedRecord` instead of `ValueAndTimestamp`? Also, the request type
Hey Matthias,
thanks for the feedback
I think if one materializes a versioned store, then the query is posed to
the versioned state store. So the type of materialized store determines the
type of store and consequently all the classes for running the query (for
example,
Thanks for updating the KIP and splitting into multiple ones. I am just
going to reply for the single-key-single-timestamp case below.
It seems the `KeyQuery.java` code snipped is "incomplete" -- the class
definition is missing.
At the same time, the example uses `VersionedKeyQuery` so I am
Hi all,
thanks to all for the great points you mentioned.
Addressed reviews are listed as follows:
1. The methods are defined as composable, as Lucas suggested. Now we have
even more types of single-key_multi-timestamp queries. As Matthias
suggested in his first review, now with composable
Seems there was a lot of additional feedback. Looking forward to an
updated version of the KIP.
I also agree to make the queries more composable. I was considering to
raise this originally, but hold off because `RangeQuery` is also not
designed very composable. But for versioned store, we
Hey Alieh,
Thanks for the KIP!
It looks like the KIP proposes three different types of interactive queries for
versioned stores, though they are grouped together into two classes:
VersionedKeyQuery adds supports for single-key, single-timestamp lookups, and
also for single-key,
Hi,
I will use the initials of the authors to distinguish the points.
LB 2.
I like the idea of composable construction of queries. It would make the
API more readable. I think this is better than the VersionsQualifier, I
proposed in BC 3..
LB 4. and LB 5.
Being explicit on the time bounds
Hi Alieh,
thanks a lot for the KIP. IQ with time semantics is going to be
another great improvement towards having crystal clear streaming
semantics!
1. I agree with Bruno and Matthias, to remove the 'bound' term for the
timestamps. It's confusing that we have bounds for both timestamps and
Thanks, Bruno, for the feedback.
- I agree with both points 2 and 3. About 3: Having "VersionsQualifier"
reduces the number of methods and makes everything less confusing. At the
end, that will be easier to use for the developers.
- About point 4: I renamed all the properties and
Thanks, Matthias, for the feedback!
> Not sure if "bound" is the right term?
My understanding from your comment is that just the name "*bound*" is not
the right one, while having this query type makes sense. Right? If *bound*
is a confusing word, we can simply change it to a better one. I do not
Hi Alieh,
Thanks for the KIP!
Here my feedback.
1.
You can remove the private fields and constructors from the KIP. Those
are implementation details.
2.
Some proposals for renamings
in VersionedKeyQuery
withKeyWithTimestampBound()
-> withKeyAndAsOf()
withKeyWithTimestampRange()
->
Thanks for the KIP Alieh. Glad to see that we can add IQ to the new
versioned stores!
Couple of questions:
single-key lookup with timestamp (upper) bound
Not sure if "bound" is the right term? In the end, it's a point lookup
for a key plus timestamps, so it's an as-of timestamp (not a
Hi all,
I would like to propose a KIP to support IQv2 for versioned state stores.
https://cwiki.apache.org/confluence/display/KAFKA/KIP-960%3A+Support+interactive+queries+%28IQv2%29+for+versioned+state+stores
Looking forward to your feedback!
Cheers,
Alieh
17 matches
Mail list logo
