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 > >> >
