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
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
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
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
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`
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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)
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
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
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
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
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
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
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?)
30 matches
Mail list logo