Hi Alieh,
Thanks for the KIP!
I think from a KIP
1.
I propose to throw an IllegalArgumentException or an
IllegalStateException for meaningless combinations. In any case, the KIP
should specify what exception is thrown.
2.
Why does not specifying a range return the latest version? I would
expect that it returns all versions since an empty lower or upper limit
is interpreted as no limit.
3.
I second Matthias comment about replacing "asOf" with "until" or "to".
4.
Do we need "allVersions()"? As I said above I would return all versions
if no limits are specified. I think if we get rid of allVersions() there
might not be any meaningless combinations anymore.
If a user applies twice the same limit like for example
MultiVersionedKeyQuery.with(key).from(t1).from(t2) the last one wins.
5.
Could you add some more examples with time ranges to the example section?
6.
The KIP misses the test plan section.
7.
I propose to rename the class to "MultiVersionKeyQuery" since we are
querying multiple versions of the same key.
8.
Could you also add withAscendingTimestamps()? IMO it gives users the
possibility to make their code more readable instead of only relying on
the default.
Best,
Bruno
On 8/17/23 4:13 AM, Matthias J. Sax wrote:
Thanks for splitting this part into a separate KIP!
For `withKey()` we should be explicit that `null` is not allowed.
(Looking into existing `KeyQuery` it seems the JavaDocs don't cover this
either -- would you like to do a tiny cleanup PR for this, or fix
on-the-side in one of your PRs?)
The key query returns all the records that are valid in the time range
starting from the timestamp {@code fromTimestamp}.
In the JavaDocs you use the phrase `are valid` -- I think we need to
explain what "valid" means? It might even be worth to add some examples.
It's annoying, but being precise if kinda important.
With regard to KIP-962, should we allow `null` for time bounds ? The
JavaDocs should also be explicit if `null` is allowed or not and what
the semantics are if allowed.
You are using `asOf()` however, because we are doing time-range queries,
to me using `until()` to describe the upper bound would sound better (I
am not a native speaker though, so maybe I am off?)
The key query returns all the records that have timestamp <= {@code
asOfTimestamp}.
This is only correct if not lower-bound is set, right?
In your reply to KIP-960 you mentioned:
the meaningless combinations are prevented by throwing exceptions.
We should add corresponding JavaDocs like:
@throws IllegalArgumentException if {@code fromTimestamp} is equal or
larger than {@code untilTimestamp}
Or something similar.
With regard to KIP-960: if we need to introduce a `VersionedKeyQuery`
class for single-key-single-ts lookup, would we need to find a new name
for the query class of this KIP, given that the return type is different?
-Matthias
On 8/16/23 10:57 AM, 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 single-key, multi-timestamp interactive queries here. You can
see all
the addressed reviews on the following page. Thanks in advance.
KIP-968: Support single-key_multi-timestamp interactive queries (IQv2)
for
versioned state stores
<https://cwiki.apache.org/confluence/display/KAFKA/KIP-968%3A+Support+single-key_multi-timestamp+interactive+queries+%28IQv2%29+for+versioned+state+stores>
I look forward to your feedback!
Cheers,
Alieh
