Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thanks! SGTM. Seems all open questions are resolved. Thanks for pushing this through! -Matthias On 11/21/23 2:29 AM, Alieh Saeedi wrote: Yes Matthias, Based on the discussion we had, it has now been changed to Optional and the default is empty (for the latest). Also, the `validTo()` method

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Yes Matthias, Based on the discussion we had, it has now been changed to Optional and the default is empty (for the latest). Also, the `validTo()` method returns an Optional. Cheers, Alieh On Mon, Nov 20, 2023 at 7:38 PM Matthias J. Sax wrote: > I think we should also discuss a little more

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

I think we should also discuss a little more about `validTo()` method? Given that "latest" version does not have a valid-to TS, should we change the return type to `Optional` and return `empty()` for "latest"? ATM the KIP uses `MAX_VALUE` for "latest" what seems to be less clean? We could

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi Alieh, Although, I've already voted, I found a minor miss. You should also add a method isDescending() since the results could also be unordered now that we agreed that the results are unordered by default. If both -- isDescending() and isAscending -- are false neither

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi all, Thank you for the feedback. So we agreed on no default ordering for keys and TSs. So I must provide both withAscendingXx() and with DescendingXx() for the class. Apart from that, I think we can either remove the existing constructor for the `VersionedRecord` class or follow the `Optional`

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thanks, Alieh. Overall SGTM. About `validTo` -- wondering if we should make it an `Optional` and set to `empty()` by default? I am totally ok with going with the 3-way option about ordering using default "undefined". For this KIP (as it's all net new) nothing really changes. -- However, we

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi all, Thank you for the feedback and the interesting solutions you suggested. The KIP and the corresponding PR are updated as follows: - Snapshot semantics is implemented in the `get(key, fromTime, toTime)` method to guarantee consistency. - The `ValueIterator` interface is replaced

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi, 80) We do not keep backwards compatibility with IQv1, right? I would even say that currently we do not need to keep backwards compatibility among IQv2 versions since we marked the API "Evolving" (do we only mean code compatibility here or also behavioral compatibility?). I propose to try

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Just catching up on this one. 50) I am also in favor of setting `validTo` in VersionedRecord for single-key single-ts lookup; it seems better to return the proper timestamp. The timestamp is already in the store and it's cheap to extract it and add to the result, and it might be valuable

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi Alieh, I agree with Bruno that a weaker guarantee could be useful already, and it's anyway possible to strengthen the guarantees later on. On the other hand, it would be nice to provide a consistent view across all segments if it doesn't come with major downsides, because until now IQ does

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi Alieh, Regarding the semantics/guarantees of the query type: Do we need a snapshot semantic or can we specify a weaker but still useful semantic? An option could be to guarantee that: 1. the query returns the latest version when the query arrived at the state store 2. the query returns a

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi, Thanks for the updates! First my take on previous comments: 50) I am in favor of deprecating the constructor that does not take the validTo parameter. That implies that I am in favor of modifying get(key, asOf) to set the correct validTo. 60) I am in favor of renaming ValueIterator

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thank you, Bruno and Matthias, for keeping the discussion going and for reviewing the PR. Here are the KIP updates: - I removed the `peek()` from the `ValueIterator` interface since we do not need it. - Yes, Bruno, the `validTo` field in the `VersionedRecod` class is exclusive. I

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi Alieh, First of all, I like the examples. Is validTo in VersionedRecord exclusive or inclusive? In the javadocs you write: "@param validTothe latest timestamp that value is valid" I think that is not true because the validity is defined by the start time of the new version. The new

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi all, @Matthias: I think Victoria was right. I must add the method `get(key, fromTime, toTime)` to the interface `VersionedKeyValueStore`. Right now, having the method only in `RocksDBVersionedStore`, made me to have an instance of `RocksDBVersionedStore` (instead of `VersionedKeyValueStore`) in

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thank you, Matthias, Bruno, and Guozhang for keeping the discussion going. Here is the list of changes I made: 1. I enriched the "Example" section as Bruno suggested. Do you please have a look at that section? I think I devised an interesting one ;-) 2. As Matthias and Guozhang

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thanks Alieh for the KIP, as well as a nice summary of all the discussions! Just my 2c regarding your open questions: 1. I just checked KIP-889 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-889%3A+Versioned+State+Stores) and we used "VersionedRecord get(K key, long asOfTimestamp);", so I

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thanks for the update! Some thoughts about changes you made and open questions you raised: 10) About asOf vs until: I was just looking into `WindowKeyQuery`, `WindowRangeQuery` and also `ReadOnlyWindowStore` interfaces. For those, we use "timeFrom" and "timeTo", so it seems best to

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thanks for the updates, Alieh! The example in the KIP uses the allVersions() method which we agreed to remove. Regarding your questions: 1. asOf vs. until: I am fine with both but slightly prefer until. 2. What about KeyMultiVersionsQuery, KeyVersionsQuery (KIP-960 would then be

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Thank you all for the very exact and constructive comments. I really enjoyed reading your ideas and all the important points you made me aware of. I updated KIP-968 as follows: 1. If the key or time bounds are null, the method returns NPE. 2. The "valid" word: I removed the sentence "all

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Bruno and I had some background conversation about the `get` prefix question including a few other committers. The official policy was never changed, and we should not add the `get`-prefix. It's a slip on our side in previous KIPs to add the `get`-prefix and we should actually clean it up

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi Matthias, Given all the IQv2 KIPs that use getX and given recent PRs (internal interfaces mainly) that got merged, I was under the impression that we moved away from the strict no-getX policy. I do not think it was an accident using getX in the IQv2 KIPs since somebody would have brought

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

I agree to (almost) everything what Bruno said. In general, we tend to move away from using getters without "get", recently. So I would keep the "get". This is new to me? Can you elaborate on this point? Why do you think that's the case? I actually did realize (after Walker mentioned it)

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi, Regarding tombstones: As far as I understand, we need to add either a validTo field to VersionedRecord or we need to return tombstones, otherwise the result is not complete, because users could never know a record was deleted at some point before the second non-null value was put. I like

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hey Alieh thanks for the KIP, Weighing in on the AsOf vs Until debate I think either is fine from a natural language perspective. Personally AsOf makes more sense to me where until gives me the idea that the query is making a change. It's totally a connotative difference and not that important. I

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

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

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

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

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

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

RE: Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

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

Re: [DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

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

[DISCUSS] KIP-968: Support single-key_multi-timestamp interactive queries (IQv2) for versioned state stores

Hi all, I splitted KIP-960 into three separate KIPs. Therefore, please continue discussions about single-key, multi-timestamp interactive