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 suggested, I renamed variables and methods
as follows:
- "fromTimestamp" -> "fromTime"
- "asOfTimestamp" -> "toTime"
- "from(Instant fromTime)" -> "fromTime(Instant fromTime)"
- "asOf(Instant toTime)" -> "toTime(Instant toTime)"
3.
About throwing an NPE when time bounds are `null`: Ok, Matthias, I am
convinced by your idea. I consider a null timestamp as "no bound". But I
had to change KIP-960 and the corresponding PR to be consistent as well.
Answering questions and some more discussion points:
1.
For now, I keep the class names as they are.
2.
About the new field "validTo" in VersionedRecord. Yes Matthias I keep
the old constructor, which does not have `validTo` as an input parameter.
But in the body of the old constructor, I consider the default value MAX
for the validTo field. I think MAX must be the default value for `validTo`
since before inserting a tombstone or a new value for the same key, the
value must be valid till iternity.
3.
Regarding changing the ValueIterator interface from `public interface
ValueIterator<V> extends Iterator<V>` to `public interface
ValueIterator<V> extends Iterator<VersionedRecord<V>>`: Matthias, I do not
know how it improves type safety because the MultiVersionedKeyQuery class
returns a ValueIterator of VersionedRecord any way. But if we want to be
consistent with KeyValueIterator, we must apply the changes you suggested.
4.
Regarding adding the new `get(key, fromTime, toTime)` method to the
public interface `VersionedKeyValueStore` or only adding it to the class
`RocksDBVersionedStore`: In the KIP, I changed the interface as Victoria
suggested. But still, I am not convinced why we do that. @Victoria: Do
you please clarify why we have to define it in the interface? More
specifically, why do we need to use generic VersionedKeyValueStore
instead of simply using the implementing classes?
Cheers,
Alieh
On Sat, Oct 14, 2023 at 3:35 AM Guozhang Wang <guozhang.wang...@gmail.com>
wrote:
> 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<V> get(K key, long asOfTimestamp);", so I
> feel to be consistent with this API is better compared with being
> consistent with "WindowKeyQuery"?
>
> 3. I agree with Matthias that naming is always tricky, and I also tend
> to be consistent with what we already have (even if in retro it may
> not be the best idea :P and if that was really becoming a complaint,
> we would change it across the board in one shot as well later).
>
> Guozhang
>
> On Wed, Oct 11, 2023 at 9:12 PM Matthias J. Sax <mj...@apache.org> wrote:
> >
> > 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 actually use
> > `to(Instant toTime)` to keep the naming consistent across the board?
> >
> > If yes, we should also do `from (Instant fromTime)` and use getters
> > `fromTime()` and `toTime()` -- given that it's range bounds it seems
> > acceptable to me, to diverge a little bit from KIP-960 `asOfTimestamp()`
> > -- but we could also rename it to `asOfTime()`? -- Given that we
> > strongly type with `Instant` I am not worried about semantic ambiguity.
> >
> >
> >
> > 20) About throwing a NPE when time bounds are `null` -- why? (For the
> > key it makes sense as is mandatory to have a key.) Could we not
> > interpret `null` as "no bound". We did KIP-941 to add `null` for
> > open-ended `RangeQueries`, so I am wondering if we should just stick to
> > the same semantics?
> >
> > Cf
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-941%3A+Range+queries+to+accept+null+lower+and+upper+bounds
> >
> >
> >
> > 30) About the class naming. That's always tricky, and I am not married
> > to my proposal. I agree with Bruno that the other suggested names are
> > not really better. -- The underlying idea was, to get some "consistent"
> > naming across the board.
> >
> > Existing `KeyQuery`
> > New `VersionedKeyQuery` (KIP-960; we add a prefix)
> > New `MultiVersionKeyQuery` (this KIP; extend the prefix with a
> pre-prefix)
> >
> > Existing `RangeQuery`
> > New `MultiVersionRangeQuery` (KIP-969; add same prefix as above)
> >
> >
> >
> > 40) I am fine with not adding `range(from, to)` -- it was just an idea.
> >
> >
> >
> >
> >
> > Some more follow up question:
> >
> > 50) You propose to add a new constructor and getter to `VersionedRecord`
> > -- I am wondering if this implies that `validTo` is optional because the
> > existing constructor is not deprecated? -- Also, what happens if
> > `validTo` is not set and `valueTo()` is called? Or do we intent to make
> > `validTo` mandatory?
> >
> > Maybe this question can only be answered when working on the code, but I
> > am wondering if we should make `validTo` mandatory or not... And what
> > the "blast radius" of changing `VersionedRecord` will be in general. Do
> > you have already some POC PR that we could look at to get some signals
> > about this?
> >
> >
> >
> > 60) The new query class is defined to return
> > `ValueIterator<VersionedRecord<V>>` -- while I like the idea to add
> > `ValueIterator<V>` in a generic way on the one hand, I am wondering if
> > it might be better to change it, and enforce its usage (ie, return type)
> > of `VersionedRecord` to improve type safety (type erasure is often a
> > pain, and we could mitigate it this way).
> >
> > Btw: We actually do a similar thing for `KeyValueIterator`.
> >
> > Ie,
> >
> > public interface ValueIterator<V> extends Iterator<ValueAndTimestamp<V>>
> >
> > and
> >
> > ValueAndTimestamp<V> peek();
> >
> > This would imply that the return type of the new query is
> > `ValueIterator<V>` on the interface what seems simpler and more elegant?
> >
> > If we go with the change, I am also wondering if we need to find a
> > better name for the new iterator class? Maybe `VersionIterator` or
> > something like this?
> >
> > Of course it might limit the use of `ValueIterator` for other value
> > types -- not sure if this a limitation that is prohibitive? My gut
> > feeling is, that is should not be too limiting.
> >
> >
> >
> >
> > 70) Do we really need the change in `VersionedKeyValueStore` and add a
> > new method? In the end, the idea of IQv2 is to avoid exactly this... It
> > was the main issue for IQv1, that the base interface of the store needed
> > an update and thus all classed implementing the base interface, making
> > it very cumbersome to add new query types. -- Of course, we need this
> > new method on the actually implementation (as private method) that can
> > be called from `query()` method, but adding it to the interface seems to
> > defeat the purpose of IQv2.
> >
> > Note, for existing IQv2 queries types that go against others stores, the
> > public methods already existed when IQv2 was introduces, and thus the
> > implementation of these query types just pragmatically re-used existing
> > methods -- but it does not imply that new public method should be added.
> >
> >
> >
> >
> > -Matthias
> >
> >
> > On 10/11/23 5:11 AM, Bruno Cadonna wrote:
> > > 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 KeyVersionQuery). However, I am also fine with
> > > MultiVersionedKeyQuery since none of the names sounds better or worse
> to
> > > me.
> > > 3. I agree with you not to introduce the method with the two bounds to
> > > keep things simple.
> > > 4. Forget about fromTime() an asOfTime(), from() and asOf() is fine.
> > > 5. The main purpose is to show how to use the API. Maybe make an
> example
> > > with just the key to distinguish this query from the single value query
> > > of KIP-960 and then one with a key and a time range. When you iterate
> > > over the results you could also call validTo(). Maybe add some actual
> > > records in the comments to show what the result might look like.
> > >
> > > Regarding the test plan, I hope you also plan to add unit tests in all
> > > of your KIPs. Maybe you could also explain why system tests are not
> > > needed here.
> > >
> > > Best,
> > > Bruno
> > >
> > > On 10/10/23 5:36 PM, Alieh Saeedi wrote:
> > >> 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 the records that
> are
> > >> valid..." and replaced it with an exact explanation. More over, I
> > >> explained
> > >> it with an example in the KIP but not in the javadocs. Do I need
> > >> to add the
> > >> example to the javadocs as well?
> > >> 3. Since I followed Bruno's suggestion and removed the
> allVersions()
> > >> method, the problem of meaningless combinations is solved, and I
> > >> do not
> > >> need any IllegalArgumentException or something like that.
> > >> Therefore, the
> > >> change is that if no time bound is specified, the query returns
> > >> the records
> > >> with the specified key for all timestamps (all versions).
> > >> 4. As Victoria suggested, adding a method to the
> > >> *VersionedKeyValueStore
> > >> *interface is essential. So I did that. I had this method only in
> the
> > >> RocksDBVersionedStore class, which was not enough.
> > >> 5. I added the *validTo* field to the VersionedRecord class to be
> > >> able
> > >> to represent the tombstones. As you suggested, we postpone solving
> > >> the
> > >> problem of retrieving consecutive tombstones for later.
> > >> 6. I added the "Test Plan" section to all KIPs. I hope what I
> > >> wrote is
> > >> convincing.
> > >> 7. I added the *withAscendingTimestamp()* method to provide more
> > >> code readability
> > >> for the user.
> > >> 8. I removed the evil word "get" from all getter methods.
> > >>
> > >> There have also been some more suggestions which I am still not
> convinced
> > >> or clear about them:
> > >>
> > >> 1. Regarding asOf vs until: reading all comments, my conclusion
> > >> was that
> > >> I keep it as "asOf" (following Walker's idea as the native speaker
> > >> as well
> > >> as Bruno's suggestion to be consistent with single-key_single_ts
> > >> queries).
> > >> But I do not have a personal preference. If you insist on "until",
> > >> I change
> > >> it.
> > >> 2. Bruno suggested renaming the class "MultiVersionedKeyQuery" to
> sth
> > >> else. We already had a long discussion about the name with
> > >> Matthias. I am
> > >> open to renaming it to something else, but do you have any ideas?
> > >> 3. Matthias suggested having a method with two input parameters
> that
> > >> enables the user to specify both time bounds in the same method.
> > >> Isn't it
> > >> introducing redundancy? It is somehow disrespectful to the idea of
> > >> having
> > >> composable methods.
> > >> 4. Bruno suggested renaming the methods "asOf" and "from" to
> > >> "asOfTime"
> > >> and "fromTime". If I do that, then it is not consistent with
> KIP-960.
> > >> Moreover, the input parameter is clearly a timestamp, which
> explains
> > >> enough. What do you think about that?
> > >> 5. I was asked to add more examples to the example section. My
> > >> question
> > >> is, what is the main purpose of that? If I know it clearly, then I
> > >> can add
> > >> what you mean.
> > >>
> > >>
> > >>
> > >> Cheers,
> > >> Alieh
> > >>
> > >> On Tue, Oct 10, 2023 at 1:13 AM Matthias J. Sax <mj...@apache.org>
> wrote:
> > >>
> > >>> 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 doing a follow up
> KIP.
> > >>>
> > >>>
> > >>> -Matthias
> > >>>
> > >>>
> > >>> On 10/5/23 5:26 AM, Bruno Cadonna wrote:
> > >>>> 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 it up, otherwise.
> > >>>>
> > >>>> I am fine with both types of getters.
> > >>>>
> > >>>> If we think, we need to discuss this in a broader context, let's
> > >>>> start a
> > >>>> separate thread.
> > >>>>
> > >>>>
> > >>>> Best,
> > >>>> Bruno
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>
> > >>>> On 10/5/23 7:44 AM, Matthias J. Sax wrote:
> > >>>>> 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) that existing
> query
> > >>>>> types use `get` prefix, but to me it seems that it was by accident
> and
> > >>>>> we should consider correcting it? Thus, I would actually prefer to
> not
> > >>>>> add the `get` prefix for new methods query types.
> > >>>>>
> > >>>>> IMHO, we should do a follow up KIP to deprecate all methods with
> `get`
> > >>>>> prefix and replace them with new ones without `get` -- it's of
> course
> > >>>>> always kinda "unnecessary" noise, but if we don't do it, we might
> get
> > >>>>> into more and more inconsistent naming what would result in a "bad"
> > >>>>> API.
> > >>>>>
> > >>>>> If we indeed want to change the convention and use the `get`
> prefix, I
> > >>>>> would strongly advocate to bit the bullet and do KIP to
> pro-actively
> > >>>>> add the `get` "everywhere" it's missing... But overall, it seems
> to be
> > >>>>> a much broader decision, and we should get buy in from many
> committers
> > >>>>> about it -- as long as there is no broad consensus to add `get`
> > >>>>> everywhere, I would strongly prefer not to diverge from the current
> > >>>>> agreement to omit `get`.
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> -Matthias
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On 10/4/23 2:36 AM, Bruno Cadonna wrote:
> > >>>>>> 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 more adding the validTo field since it makes the result
> more
> > >>>>>> concise and easier interpretable.
> > >>>>>>
> > >>>>>> Extending on Victoria's example, with the following puts
> > >>>>>>
> > >>>>>> put(k, v1, time=0)
> > >>>>>> put(k, null, time=5)
> > >>>>>> put(k, null, time=10)
> > >>>>>> put(k, null, time=15)
> > >>>>>> put(k, v2, time=20)
> > >>>>>>
> > >>>>>> the result with tombstones would be
> > >>>>>>
> > >>>>>> value, timestamp
> > >>>>>> (v1, 0)
> > >>>>>> (null, 5)
> > >>>>>> (null, 10)
> > >>>>>> (null, 15)
> > >>>>>> (v2, 20)
> > >>>>>>
> > >>>>>> instead of
> > >>>>>>
> > >>>>>> value, timestamp, validTo
> > >>>>>> (v1, 0, 5)
> > >>>>>> (v2, 20, null)
> > >>>>>>
> > >>>>>> The benefit of conciseness would already apply to one single
> > >>>>>> tombstone.
> > >>>>>>
> > >>>>>> On the other hand, why would somebody write consecutive tombstones
> > >>>>>> into a versioned state store? I guess if somebody does that on
> > >>>>>> purpose, then there should be a way to retrieve each of those
> > >>>>>> tombstones, right?
> > >>>>>> So maybe we need both -- validTo field and the option to return
> > >>>>>> tombstones. The latter might be moved to a future KIP in case we
> see
> > >>>>>> the need.
> > >>>>>>
> > >>>>>>
> > >>>>>> Regarding .within(fromTs, toTs):
> > >>>>>> I would keep it simple with .from() and .asOfTimestamp() (or
> > >>>>>> .until()). If we go with .within(), I would opt for
> > >>>>>> .withinTimeRange(fromTs, toTs), because the query becomes more
> > >>> readable:
> > >>>>>>
> > >>>>>> MultiVersionedKeyQuery
> > >>>>>> .withKey(1)
> > >>>>>> .withinTimeRange(Instant.parse(2023-08-03T10:37:30.00Z),
> > >>>>>> Instant.parse(2023-08-04T10:37:30.00Z))
> > >>>>>>
> > >>>>>> If we stay with .from() and .until(), we should consider
> .fromTime()
> > >>>>>> and .untilTime() (or .toTime()):
> > >>>>>>
> > >>>>>> MultiVersionedKeyQuery
> > >>>>>> .withKey(1)
> > >>>>>> .fromTime(Instant.parse(2023-08-03T10:37:30.00Z))
> > >>>>>> .untilTime(Instant.parse(2023-08-04T10:37:30.00Z))
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> Regarding asOf vs. until:
> > >>>>>> I think asOf() is more used in point in time queries as Walker
> > >>>>>> mentioned where this KIP specifies a time range. IMO asOf() fits
> very
> > >>>>>> well with KIP-960 where one version is queried, but here I think
> > >>>>>> .until() fits better. That might just be a matter of taste and in
> the
> > >>>>>> end I am fine with both as long as it is well documented.
> > >>>>>>
> > >>>>>>
> > >>>>>> Regarding getters without "get":
> > >>>>>> In the other IQv2 classes we used getters with "get". In general,
> we
> > >>>>>> tend to move away from using getters without "get", recently. So I
> > >>>>>> would keep the "get".
> > >>>>>>
> > >>>>>>
> > >>>>>> Best,
> > >>>>>> Bruno
> > >>>>>>
> > >>>>>> On 10/3/23 7:49 PM, Walker Carlson wrote:
> > >>>>>>> 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 think as of is
> > >>>>>>> pretty
> > >>>>>>> frequently used in point of time queries.
> > >>>>>>>
> > >>>>>>> Also for these methods it makes sense to drop the "get" We don't
> > >>>>>>> normally use that in getters
> > >>>>>>>
> > >>>>>>> * The key that was specified for this query.
> > >>>>>>> */
> > >>>>>>> public K getKey();
> > >>>>>>>
> > >>>>>>> /**
> > >>>>>>> * The starting time point of the query, if specified
> > >>>>>>> */
> > >>>>>>> public Optional<Instant> getFromTimestamp();
> > >>>>>>>
> > >>>>>>> /**
> > >>>>>>> * The ending time point of the query, if specified
> > >>>>>>> */
> > >>>>>>> public Optional<Instant> getAsOfTimestamp();
> > >>>>>>>
> > >>>>>>> Other than that I didn't have too much to add. Overall I like the
> > >>>>>>> direction
> > >>>>>>> of the KIP and think the funcatinlyt is all there!
> > >>>>>>> best,
> > >>>>>>> Walker
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On Mon, Oct 2, 2023 at 10:46 PM Matthias J. Sax <
> mj...@apache.org>
> > >>>>>>> wrote:
> > >>>>>>>
> > >>>>>>>> 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
> > >>>>>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>
> > >>
>
