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 <mj...@apache.org> wrote:

> 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 also use `-1` (unknown), but both might be less expressive than
> `Optional`?
>
>
> -Matthias
>
> On 11/20/23 1:59 AM, Bruno Cadonna wrote:
> > 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
> > withDescendingTimestamps() nor withAscendingTimestamps() was called.
> >
> > Best,
> > Bruno
> >
> > On 11/17/23 11:25 AM, Alieh Saeedi wrote:
> >> 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` thing.
> >>
> >> Since many hidden aspects of the KIP are quite clear now and we have
> come
> >> to a consensus about them, I think it 's time to vote ;-)
> >> I look forward to your votes. Thanks a lot.
> >>
> >> Cheers,
> >> Alieh
> >>
> >> On Fri, Nov 17, 2023 at 2:27 AM Matthias J. Sax <mj...@apache.org>
> wrote:
> >>
> >>> 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 should amend `RangeQuery`/KIP-985 to align it.
> >>>
> >>> Btw: so far we focused on key-ordering, but I believe the same
> "ordering
> >>> undefined by default" would apply to time-ordering, too? This might
> >>> affect KIP-997, too.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 11/16/23 12:51 AM, Bruno Cadonna wrote:
> >>>> 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
> >>>> to not limit ourselves for backwards compatibility that we explicitly
> >>>> marked as evolving.
> >>>> I re-read the discussion on KIP-985. In that discussion, we were quite
> >>>> focused on what the state store provides. I see that for range
> queries,
> >>>> we have methods on the state store interface that specify the order,
> >>>> but
> >>>> that should be kind of orthogonal to the IQv2 query type. Let's assume
> >>>> somebody in the future adds a state store implementation that is not
> >>>> order based. To account for use cases where the order does not matter,
> >>>> this person might also add a method to the state store interface that
> >>>> does not guarantee any order. However, our range query type is
> >>>> specified
> >>>> to guarantee order by default. So we need to add something like
> >>>> withNoOrder() to the query type to allow the use cases that does not
> >>>> need order and has the better performance in IQ. That does not look
> >>>> very
> >>>> nice to me. Having the no-order-guaranteed option does not cost us
> >>>> anything and it keeps the IQv2 interface flexible. I assume we want to
> >>>> drop the Evolving annotation at some point.
> >>>> Sorry for not having brought this up in the discussion about KIP-985.
> >>>>
> >>>> Best,
> >>>> Bruno
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On 11/15/23 6:56 AM, Matthias J. Sax wrote:
> >>>>> 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
> information
> >>>>> for the user. Not sure though if we should deprecate the existing
> >>>>> constructor though, because for "latest" it's convenient to have?
> >>>>>
> >>>>>
> >>>>> 60) Yes, I meant `VersionedRecord`. Sorry for the mixup.
> >>>>>
> >>>>>
> >>>>> 80) We did discuss this question on KIP-985 (maybe you missed it
> >>>>> Bruno). It's kinda tricky.
> >>>>>
> >>>>> Historically, it seems that IQv1, ie, the `ReadOnlyXxx` interfaces
> >>>>> provide a clear contract that `range()` is ascending and
> >>>>> `reverseRange()` is descending.
> >>>>>
> >>>>> For `RangeQuery`, the question is, if we did implicitly inherit this
> >>>>> contract? Our conclusion on KIP-985 discussion was, that we did
> >>>>> inherit it. If this holds true, changing the contract would be a
> >>>>> breaking change (what might still be acceptable, given that the
> >>>>> interface is annotated as unstable, and that IQv2 is not widely
> >>>>> adopted yet). I am happy to go with the 3-option contract, but just
> >>>>> want to ensure we all agree it's the right way to go, and we are
> >>>>> potentially willing to pay the price of backward incompatibility.
> >>>>>
> >>>>>
> >>>>>
> >>>>>> Do we need a snapshot semantic or can we specify a weaker but still
> >>>>>> useful semantic?
> >>>>>
> >>>>> I don't think we necessarily need it, but as pointed out by Lucas,
> all
> >>>>> existing queries provide it. Overall, my main point is really about
> >>>>> not implementing something "random", but defining a proper binding
> >>>>> contract that allows users to reason about it.
> >>>>>
> >>>>> I general, I agree that weaker semantics might be sufficient, but I
> am
> >>>>> not sure if we can implement anything weaker in a reasonable way?
> >>>>> Happy to be convinced otherwise. (I have some example, that I will
> >>>>> omit for now, as I hope we can actually go with snapshot semantics.)
> >>>>>
> >>>>> The RocksDB Snaptshot idea from Lucas sounds very promising. I was
> not
> >>>>> aware that we could do this with RocksDB (otherwise I might have
> >>>>> suggested it on the PR right away). I guess the only open question
> >>>>> remaining would be, if we can provide the same guarantees for a
> future
> >>>>> in-memory implementation for VersionedStores? It sounds possible to
> >>>>> do, but we should have some level of confidence about it?
> >>>>>
> >>>>>
> >>>>> -Matthias
> >>>>>
> >>>>> On 11/14/23 6:33 AM, Lucas Brutschy wrote:
> >>>>>> 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 provide a consistent view (via iterators), and this would be
> the
> >>>>>> first feature that diverges from that guarantee.
> >>>>>>
> >>>>>> I think a consistent could be achieved relatively easily by
> >>>>>> creating a
> >>>>>> snapshot (https://github.com/facebook/rocksdb/wiki/Snapshot) that
> >>>>>> will
> >>>>>> ensure a consistent view on a single DB and using
> >>>>>> `ReadOptions.setSnapshot` for the gets. Since versioned state stores
> >>>>>> segments seem to be backed by a single RocksDB instance (that was
> >>>>>> unclear to me during our earlier discussion), a single snapshot
> >>>>>> should
> >>>>>> be enough to guarantee a consistent view - we just need to make sure
> >>>>>> to clean up the snapshots after use, similar to iterators. If we
> >>>>>> instead need a consistent view across multiple RocksDB instances, we
> >>>>>> may have to acquire a write lock on all of those (could use the
> >>>>>> current object monitors of our `RocksDB` interface) for the duration
> >>>>>> of creating snapshots across all instances - which I think would
> also
> >>>>>> be permissible performance-wise. Snapshots are just a sequence
> number
> >>>>>> and should be pretty lightweight to create (they have, however,
> >>>>>> downside when it comes to compaction just like iterators).
> >>>>>>
> >>>>>> With that in mind, I would be in favor of at least exploring the
> >>>>>> option of using snapshots for a consistent view here, before
> dropping
> >>>>>> this useful guarantee.
> >>>>>>
> >>>>>> Cheers,
> >>>>>> Lucas
> >>>>>>
> >>>>>> On Tue, Nov 14, 2023 at 2:20 PM Bruno Cadonna <cado...@apache.org>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> 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 valid history, i.e., versions with
> >>>>>>> adjacent and
> >>>>>>> non-overlapping validity intervals.
> >>>>>>>
> >>>>>>> I think guarantee 1 is quite obvious. Guarantee 2 maybe needs some
> >>>>>>> explanation. If we do not avoid writes to the state store during
> the
> >>>>>>> processing of interactive queries, it might for example happen that
> >>> the
> >>>>>>> latest version in the state store moves to data structures that are
> >>>>>>> responsible for older versions. In our RocksDB implementation
> >>>>>>> that are
> >>>>>>> the segments. Thus, it could be that during query processing Kafka
> >>>>>>> Streams reads the latest value x and encounters again x in a
> segment
> >>>>>>> because a processor put a newer version of x in the versioned state
> >>>>>>> store. A similar scenario might also happen to earlier versions. If
> >>>>>>> Streams does not account for such cases it could return invalid
> >>>>>>> histories.
> >>>>>>>
> >>>>>>> Maybe such weaker guarantees are enough for most use cases.
> >>>>>>>
> >>>>>>> You could consider implementing weaker guarantees as I described
> and
> >>> if
> >>>>>>> there is demand propose stricter guarantees in a follow-up KIP.
> >>>>>>>
> >>>>>>> Maybe there are also other simpler guarantees that make sense.
> >>>>>>>
> >>>>>>> Best,
> >>>>>>> Bruno
> >>>>>>>
> >>>>>>>
> >>>>>>> On 11/9/23 12:30 PM, Bruno Cadonna wrote:
> >>>>>>>> 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 to VersionedRecordIterator
> >>> and
> >>>>>>>> define it as:
> >>>>>>>>
> >>>>>>>> public interface VersionedRecordIterator<V> extends
> >>>>>>>> Iterator<VersionedRecord<V>>
> >>>>>>>>
> >>>>>>>> (Matthias, you mixed up ValueAndTimestamp with VersionedRecord in
> >>> your
> >>>>>>>> last e-mail, didn't you? Just double-checking if I understood what
> >>> you
> >>>>>>>> are proposing.)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 70)
> >>>>>>>> I agree with Matthias that adding a new method on the
> >>>>>>>> VersionedKeyValueStore interface defeats the purpose of one of the
> >>>>>>>> goals
> >>>>>>>> of IQv2, i.e., not to need to extend the state store interface for
> >>>>>>>> IQ. I
> >>>>>>>> would say if we do not need the method in normal processing, we
> >>> should
> >>>>>>>> not extend the public state store interface. BTW, nobody forces
> you
> >>> to
> >>>>>>>> StoreQueryUtils. I think that internal utils class was
> >>>>>>>> introduced for
> >>>>>>>> convenience to leverage existing methods on the state store
> >>> interface.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 80)
> >>>>>>>> Why do we limit ourselves by specifying a default order on the
> >>> result?
> >>>>>>>> Different state store implementation might have different
> >>>>>>>> strategies to
> >>>>>>>> store the records which affects the order in which the records are
> >>>>>>>> returned if they are not sorted before they are returned to the
> >>>>>>>> user.
> >>>>>>>> Some users might not be interested in an order of the result and
> so
> >>>>>>>> there is no reason those users pay the cost for sorting. I
> >>>>>>>> propose to
> >>>>>>>> not specify a default order but sort the results (if needed) when
> >>>>>>>> withDescendingX() and withAscendingX() is specified on the query
> >>>>>>>> object.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Regarding the snapshot guarantees for the iterators, I need to
> >>>>>>>> think
> >>> a
> >>>>>>>> bit more about it. I will come back to this thread in the next
> >>>>>>>> days.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Best,
> >>>>>>>> Bruno
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 11/8/23 5:30 PM, Alieh Saeedi wrote:
> >>>>>>>>> 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 updated the javadocs for that.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Very important critical open questions. I list them here based on
> >>>>>>>>> priority
> >>>>>>>>> (descendingly).
> >>>>>>>>>
> >>>>>>>>>       - I implemented the `get(key, fromtime, totime)` method
> here
> >>>>>>>>>
> >>>>>>>>> <
> >>>
> https://github.com/aliehsaeedii/kafka/blob/9578b7cb7cdade22cc63f671693f5aeb993937ca/streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBVersionedStore.java#L262
> >>>> :
> >>>>>>>>>       the problem is that this implementation does not guarantee
> >>>>>>>>> consistency
> >>>>>>>>>       because processing might continue interleaved (no snapshot
> >>>>>>>>> semantics is
> >>>>>>>>>       implemented). More over, it materializes all results in
> >>>>>>>>> memory.
> >>>>>>>>>          - Solution 1: Use a lock and release it after
> >>>>>>>>> retrieving all
> >>>>>>>>> desired
> >>>>>>>>>          records from all segments.
> >>>>>>>>>             - positive point: snapshot semantics is implemented
> >>>>>>>>>             - negative points: 1) It is expensive since iterating
> >>>>>>>>> over all
> >>>>>>>>>             segments may take a long time. 2) It still requires
> >>>>>>>>> materializing results
> >>>>>>>>>             on memory
> >>>>>>>>>          - Solution 2: use `RocksDbIterator`.
> >>>>>>>>>             - positive points: 1) It guarantees snapshot
> segments.
> >>>>>>>>> 2) It
> >>>>>>>>> does
> >>>>>>>>>             not require materializing results in memory.
> >>>>>>>>>             - negative points: it is expensive because, anyway,
> we
> >>>>>>>>> need to
> >>>>>>>>>             iterate over all (many) segments.
> >>>>>>>>>
> >>>>>>>>>               Do you have any thoughts on this issue? (ref:
> >>> Matthias's
> >>>>>>>>> comment
> >>>>>>>>> <
> >>>
> https://github.com/apache/kafka/pull/14626#pullrequestreview-1709280589>)
> >>>>>>>>>
> >>>>>>>>>       - I added the field `validTo` in `VersionedRecord`. Its
> >>>>>>>>> default
> >>>>>>>>> value is
> >>>>>>>>>       MAX. But as Matthias mentioned, for the single-key
> single-ts
> >>>>>>>>>       (`VersionedKeyQuery` in KIP-960), it may not always be
> true.
> >>>>>>>>> If the
> >>>>>>>>>       returned record belongs to an old segment, maybe it is not
> >>> valid
> >>>>>>>>> any more.
> >>>>>>>>>       So MAX is not the correct value for `ValidTo`. Two
> solutions
> >>>>>>>>> come
> >>>>>>>>> to mind:
> >>>>>>>>>          - Solution 1: make the `ValidTo` as an `Optional` and
> set
> >>>>>>>>> it to
> >>>>>>>>>          `empty` for the retuned result of `get(key,
> >>>>>>>>> asOfTimestamp)`.
> >>>>>>>>>          - Solution 2: change the implementation of `get(key,
> >>>>>>>>> asOfTimestamp)`
> >>>>>>>>>          so that it finds the correct `validTo` for the returned
> >>>>>>>>> versionedRecord.
> >>>>>>>>>
> >>>>>>>>>          - In this KIP and the next one, even though the default
> >>>>>>>>> ordering is
> >>>>>>>>>       with ascending ts, I added the method
> >>>>>>>>> `withAscendingTimestamps()`
> >>>>>>>>> to have
> >>>>>>>>>       more user readibility (as Bruno suggested), while Hanyu did
> >>>>>>>>> only add
> >>>>>>>>>       `withDescending...` methods (he did not need ascending
> >>>>>>>>> because
> >>>>>>>>> that's the
> >>>>>>>>>       default anyway). Matthias believes that we should not have
> >>>>>>>>>       inconsistencies (he actually hates them:D). Shall I
> >>>>>>>>> change my
> >>>>>>>>> KIP
> >>>>>>>>> or Hanyu?
> >>>>>>>>>       Thoughts?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> That would be maybe helpful to look into the PR
> >>>>>>>>> <https://github.com/apache/kafka/pull/14626> for more clarity
> and
> >>>>>>>>> even
> >>>>>>>>> review that ;-)
> >>>>>>>>>
> >>>>>>>>> Cheers,
> >>>>>>>>> Alieh
> >>>>>>>>>
> >>>>>>>>> On Thu, Nov 2, 2023 at 7:13 PM Bruno Cadonna <cado...@apache.org
> >
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> Hi Alieh,
> >>>>>>>>>>
> >>>>>>>>>> First of all, I like the examples.
> >>>>>>>>>>
> >>>>>>>>>> Is validTo in VersionedRecord exclusive or inclusive?
> >>>>>>>>>> In the javadocs you write:
> >>>>>>>>>>
> >>>>>>>>>> "@param validTo    the 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 and the old version cannot
> >>>>>>>>>> both be
> >>>>>>>>>> valid at that same time.
> >>>>>>>>>>
> >>>>>>>>>> Theoretically, you could set validTo to the start time of the
> new
> >>>>>>>>>> version - 1. However, what is the unit of the 1? Is it
> >>>>>>>>>> nanoseconds?
> >>>>>>>>>> Milliseconds? Seconds? Sure we could agree on one, but I think
> it
> >>> is
> >>>>>>>>>> more elegant to just make the validTo exclusive. Actually, you
> >>>>>>>>>> used it
> >>>>>>>>>> as exclusive in your examples.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> Thanks for the KIP!
> >>>>>>>>>>
> >>>>>>>>>> Best,
> >>>>>>>>>> Bruno
> >>>>>>>>>>
> >>>>>>>>>> On 11/1/23 9:01 PM, Alieh Saeedi wrote:
> >>>>>>>>>>> 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 `StoreQueryUtils.runMultiVersionedKeyQuery()` method. In
> >>>>>>>>>>> future, we
> >>>>>>>>>> are
> >>>>>>>>>>> going to use the same method for In-memory/SPDB/blaBla
> versioned
> >>>>>>>>>>> stores.
> >>>>>>>>>>> Then either this method won't work any more, or we have to add
> >>>>>>>>>>> code (if
> >>>>>>>>>>> clauses) for each type of versioned stores. What do you think
> >>> about
> >>>>>>>>>>> that?
> >>>>>>>>>>>
> >>>>>>>>>>> Bests,
> >>>>>>>>>>> Alieh
> >>>>>>>>>>>
> >>>>>>>>>>> On Tue, Oct 24, 2023 at 10:01 PM Alieh Saeedi
> >>>>>>>>>>> <asae...@confluent.io>
> >>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> 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
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>
> >>>
> >>
>

Reply via email to