Hi all, Thank you for the feedback and the interesting solutions you suggested.
The KIP and the corresponding PR are updated as follows: - Snapshot semantics is implemented in the `get(key, fromTime, toTime)` method to guarantee consistency. - The `ValueIterator` interface is replaced with `VersionedRecordIterator<V> implements Iterator<VersionedRecord<V>>`. Consequently, the `MultiVersionedKeyQuery` class is modified to ` MultiVersionedKeyQuery<K, V> implements Query<VersionedRecordIterator<V >>`. - The former `get(key,ts)` method is updated so that it returns records with their `validTo` timestamps as well. I still kept the old constructor of the `VersionedRecord` class with only two parameters. The `validTo` field is initialized by `MAX` as default, but I think the default value for the `validTo` field must be `*PUT_RETURN_CODE_VALID_TO_UNDEFINED*` which is defined in the `VersionedKeyValueStore` interface and is actually `-1`. Am I right? Open question: - It seems like we do not have a conclusion about the default ordering issue. Cheers, Alieh On Thu, Nov 16, 2023 at 9:56 AM Bruno Cadonna <cado...@apache.org> 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 > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >> >
