Hi Alieh,
Thanks for the KIP!
Here my feedback.
1.
You can remove the private fields and constructors from the KIP. Those
are implementation details.
2.
Some proposals for renamings
in VersionedKeyQuery
withKeyWithTimestampBound()
-> withKeyAndAsOf()
withKeyWithTimestampRange()
-> withKeyAndTimeRange()
in VersionedRangeQuery
KeyRangeWithTimestampBound()
-> withKeyRangeAndAsOf()
withLowerBoundWithTimestampBound()
-> withLowerBoundAndAsOf()
withUpperBoundWithTimestampBound()
-> withUpperBoundAndAsOf()
withNoBoundWithTimestampBound()
-> withNoBoundsAndAsOf
keyRangeWithTimestampRange()
-> withKeyRangeAndTimeRange()
withLowerBoundWithTimestampRange()
-> withLowerBoundAndTimeRange()
withUpperBoundWithTimestampRange()
-> withUpperBounfAndTimeRange()
withNoBoundWithTimestampRange()
-> withNoBoundsAndTimeRange()
3.
Would it make sense to merge
withKeyLatestValue(final K key)
and
withKeyAllVersions(final K key)
into
withKey(final K key, final VersionsQualifier versionsQualifier)
where VersionsQualifier is an enum with values (ALL, LATEST). We could
also add EARLIEST if we feel it might be useful.
Same applies to all methods that end in LatestValue or AllVersions
4.
I think getAsOfTimestamp() should not return the lower bound. If I query
a version as of a timestamp then the query should return the latest
version less than the timestamp.
I propose to rename the getters to getTimeFrom() and getTimeTo() as in
WindowRangeQuery.
5.
Please add the Test Plan section.
Regarding long vs Instant: Did we miss to use Instant instead of long
for all interfaces of the versioned state stores?
Best,
Bruno
On 7/26/23 11:40 PM, Matthias J. Sax wrote:
Thanks for the KIP Alieh. Glad to see that we can add IQ to the new
versioned stores!
Couple of questions:
single-key lookup with timestamp (upper) bound
Not sure if "bound" is the right term? In the end, it's a point lookup
for a key plus timestamps, so it's an as-of timestamp (not a bound)? Of
course, the returned record would most likely have a different (smaller)
timestamp, but that's expected but does not make the passed in timestamp
a "bound" IMHO?
single-key query with timestamp range
single-key all versions query
Should we also add `withLowerTimeBound` and `withUpperTimeBound`
(similar to what `RangeQuery` has)?
Btw: I think we should not pass `long` for timestamps, but `Instance`
types.
For time-range queries, do we iterate over the values in timestamp
ascending order? If yes, the interface should specify it? Also, would it
make sense to add reverse order (also ok to exclude and only do if there
is demand in a follow up KIP; if not, please add to "Rejected
alternatives" section).
Also, for time-range query, what are the exact bound for stuff we
include? In the end, a value was a "valid range" (conceptually), so do
we include a record if it's valid range overlaps the search time-range,
or must it be fully included? Or would we only say, that the `validFrom`
timestamp that is stored must be in the search range (what implies that
the lower end would be a non-overlapping but "fully included" bound,
while the upper end would be a overlapping bound).
For key-range / time-range queries: do we return the result in `<k,ts>`
order or `<ts,k>` order? Also, what about reverse iterators?
About ` ValueIterator` -- think the JavaDocs have c&p error in it for
`peekNextRecord` (also, should it be called `peekNextValue`? (Also some
other JavaDocs seem to be incomplete and not describe all parameters?)
Thanks.
-Matthias
On 7/26/23 7:24 AM, Alieh Saeedi wrote:
Hi all,
I would like to propose a KIP to support IQv2 for versioned state stores.
https://cwiki.apache.org/confluence/display/KAFKA/KIP-960%3A+Support+interactive+queries+%28IQv2%29+for+versioned+state+stores
Looking forward to your feedback!
Cheers,
Alieh
