Thanks for the update.
To retrieve
the latest value(s), the user must call just the asOf method with the MAX
value (asOf(MAX)). The same applies to KIP-968. Do you think it is clumsy,
Matthias?
Well, in KIP-968 calling `asOf` and passing in a timestamp is optional,
and default is "latest", right? So while `asOf(MAX)` does the same
thing, practically users would never call `asOf` for a "latest" query?
In this KIP, we enforce that users give us a key range (we have the 4
static entry point methods to define a query for this), and we say we
default to "no bounds" for time range by default.
The existing `RangeQuery` allows to query a range of keys for existing
stores. It seems to be a common pattern to query a key-range on latest.
-- in the current proposal, users would need to do:
MultiVersionedRangeQuery.withKeyRange(startKey, endKey).asOf(MAX);
Would like to hear from others if we think that's good user experience?
If we agree to accept this, I think we should explain how to do this in
the JavaDocs (and also regular docs... --- otherwise, I can already
anticipate user question on all question-asking-channels how to do a
"normal key range query". IMHO, the problem is not that the code itself
it too clumsy, but that it's totally not obvious to uses how to express
it without actually explaining it to them. It basically violated the API
design rule "make it easy to use / simple things should be easy".
Btw: We could also re-use `RangeQuery` and add am implementation to
`VersionedStateStore` to just accept this query type, with "key range
over latest" semantics. -- The issue is of course, that uses need to
know that the query would return `ValueAndTimestamp` and not plain `V`
(or we add a translation step to unwrap the value, but we would lose the
"validFrom" timestamp -- validTo would be `null`). Because type safety
is a general issue in IQv2 it would not make it worse (in the strict
sense), but I am also not sure if we want to dig an even deeper hole...
-Matthias
On 10/10/23 11:55 AM, Alieh Saeedi wrote:
Thanks, Matthias and Bruno, for the feedback on KIP-969. Here is a summary
of the updates I made to the KIP:
1. I liked the idea of renaming methods as Matthias suggested.
2. I removed the allversions() method as I did in KIP-968. To retrieve
the latest value(s), the user must call just the asOf method with the MAX
value (asOf(MAX)). The same applies to KIP-968. Do you think it is clumsy,
Matthias?
3. I added a method to the *VersionedKeyValueStore *interface, as I did
for KIP-968.
4. Matthias: I do not get what you mean by your second comment. Isn't
the KIP already explicit about that?
> I assume, results are returned by timestamp for each key. The KIP
should be explicit about it.
Cheers,
Alieh
On Tue, Oct 3, 2023 at 6:07 AM Matthias J. Sax <mj...@apache.org> wrote:
Thanks for updating the KIP.
Not sure if I agree or not with Bruno's idea to split the query types
further? In the end, we split them only because there is three different
return types: single value, value-iterator, key-value-iterator.
What do we gain by splitting out single-ts-range-key? In the end, for
range-ts-range-key the proposed class is necessary and is a superset
(one can set both timestamps to the same value, for single-ts lookup).
The mentioned simplification might apply to "single-ts-range-key" but I
don't see a simplification for the proposed (and necessary) query type?
On the other hand, I see an advantage of a single-ts-range-key for
querying over the "latest version" with a range of keys. For a
single-ts-range-key query, this it would be the default (similar to
VersionedKeyQuery with not asOf-timestamped defined).
In the current version of the KIP, (if we agree that default should
actually return "all versions" not "latest" -- this default was
suggested by Bruno on KIP-968 and makes sense to me, so we would need to
have the same default here to stay consistent), users would need to pass
in `from(Long.MAX).to(Long.MAX)` (if I got this right) to query the
latest point in time only, what seems to be clumsy? Or we could add a
`lastestKeyOnly` option to `MultiVersionedRangeQuery`, but it does seems
a little clumsy, too.
The overall order of the returned records is by Key
I assume, results are returned by timestamp for each key. The KIP should
be explicit about it.
To be very explicit, should we rename the methods to specify the key bound?
- withRange -> withKeyRange
- withLowerBound -> withLowerKeyBound
- withUpperBound -> withUpperKeyBound
- withNoBounds -> allKeys (or withNoKeyBounds, but we use
`allVersions` and not `noTimeBound` and should align the naming?)
-Matthias
On 9/6/23 5:25 AM, Bruno Cadonna wrote:
Hi Alieh,
Thanks for the KIP!
One high level comment/question:
I assume you separated single key queries into two classes because
versioned key queries return a single value and multi version key
queries return iterators. Although, range queries always return
iterators, it would make sense to also separate range queries for
versioned state stores into range queries that return one single version
of the keys within a range and range queries that return multiple
version of the keys within a range, IMO. That would reduce the
meaningless combinations.
WDYT?
Best,
Bruno
On 8/16/23 8:01 PM, Alieh Saeedi wrote:
Hi all,
I splitted KIP-960
<
https://cwiki.apache.org/confluence/display/KAFKA/KIP-960%3A+Support+single-key_single-timestamp+interactive+queries+%28IQv2%29+for+versioned+state+stores
into
three separate KIPs. Therefore, please continue discussions about range
interactive queries here. You can see all the addressed reviews on the
following page. Thanks in advance.
KIP-969: Support range interactive queries (IQv2) for versioned state
stores
<
https://cwiki.apache.org/confluence/display/KAFKA/KIP-969%3A+Support+range+interactive+queries+%28IQv2%29+for+versioned+state+stores
I look forward to your feedback!
Cheers,
Alieh
