Hi Alieh,
Thanks for the KIP!
1. As mentioned in the discussion thread for KIP-960, I think it'd be good to
add a new method to the VersionedKeyValueStore interface for supporting
single-key, multi-timestamp lookups, as part of implementing these new
interactive queries so that the IQ implementation can simply call the relevant
underlying method.
2. Also, I'd like to discuss whether it'd be better to update the
VersionedRecord class to allow null values, or to add a validTo timestamp field
to VersionedRecord instead. Here's an example scenario to consider: suppose a
user puts two values and two tombstones into a versioned store, all for the
same key:
put(k, v1, time=0)
put(k, null, time=5)
put(k, null, time=10)
put(k, v2, time=20)
And then issues an interactive query for all records for this key, in order to
get back a `ValueIterator<VersionedRecord<V>>`. How many records do you expect
to be in ValueIterator?
By proposing to allow null values in VersionedRecord, I think you are proposing
either four records ((v1, t=0), (null, t=5), (null, t=10), (v2, t=20)) or three
records ((v1, t=0), (null, t=5), (v2, t=20)) but there's a case to be made that
actually only two records should be returned ((v1, t=0, validTo=5), (v2, t=20,
validTo=null/infinity)). The last interpretation is most consistent with the
fact that `get(key)` and `get(key, asOfTimestamp)` do not distinguish between
tombstones and no records having been inserted. As in, get(key=k,
asOfTimestamp=7) returns null instead of a VersionedRecord with null value and
timestamp 5. Curious to hear what others think about this.
Best,
Victoria
On 2023/08/17 02:13:24 "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
> >
>
