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