Thanks for the updated KIP. Overall I like it.
Victoria raises a very good point, and I personally tend to prefer (I
believe so does Victoria, but it's not totally clear from her email) if
a range query would not return any tombstones, ie, only two records in
Victoria's example. Thus, it seems best to include a `validTo` ts-field
to `VersionedRecord` -- otherwise, the retrieved result cannot be
interpreted correctly.
Not sure what others think about it.
I would also be open to actually add a `includeDeletes()` (or
`includeTombstones()`) method/flag (disabled by default) to allow users
to get all tombstone: this would only be helpful if there are two
consecutive tombstone though (if I got it right), so not sure if we want
to add it or not -- it seems also possible to add it later if there is
user demand for it, so it might be a premature addition as this point?
Nit:
the public interface ValueIterator is used
"is used" -> "is added" (otherwise it sounds like as if `ValueIterator`
exist already)
Should we also add a `.within(fromTs, toTs)` (or maybe some better
name?) to allow specifying both bounds at once? The existing
`RangeQuery` does the same for specifying the key-range, so might be
good to add for time-range too?
-Matthias
On 9/6/23 5:01 AM, Bruno Cadonna wrote:
In my last e-mail I missed to finish a sentence.
"I think from a KIP"
should be
"I think the KIP looks good!"
On 9/6/23 1:59 PM, Bruno Cadonna wrote:
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
