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