Hi Bill, Thanks for the review. I agree this needed to be nailed down.
- Jess On Fri, Jun 12, 2026 at 2:41 PM Bill Bejeck <[email protected]> wrote: > Hi Jess, > > Thanks for the KIP. > > I'd second the importance of getting the details flushed out in Matthias's > 4th comment with regards to session stores > and how we'll support `ValueTimestampeHeader` and `AggregationWithHeaders` > and if session-header-stores are supported in this KIP. > > -Bill > > On Thu, Jun 11, 2026 at 8:59 PM Matthias J. Sax <[email protected]> wrote: > > > Thanks for the KIP. Very nice to see that IQv2 gets more love. > > > > > > Couple of comments/questions. > > > > > > MJS1: The KIP says. > > > > > surfaced through the public wrapper type ValueTimestampHeaders<V> > > > > Yes, but session-header-store actually uses `AggregationWithHeaders`. > > Should we mention both cases? Or do we not include session-headers > > stores? Later, the KIP only mentions > > `MeteredTimestampedKeyValueStoreWithHeaders` and > > `MeteredTimestampedWindowStoreWithHeaders`, so maybe that's indeed what > > is proposed? If yes, why do exclude session-header-store? Also if, yes, > > would be good to call this out explicitly. > > > > > > > > MJS2: The KIP says. > > > > > The existing query types — TimestampedKeyQuery, TimestampedRangeQuery, > > WindowKeyQuery, and WindowRangeQuery — all return values without headers, > > even when run against a header-aware store. > > > > This sound as if these query types are already supported, but I believe > > they are not. Same for their timestamp-less equivalents like `KeyQuery`. > > > > I think it would be a good extension to the KIP, to also add support for > > these existing query types on the newly added header-stores. We don't > > need to define any new classed. The KIP would just say, that we extend > > the kv-ts-header store to also support existing KeyQuery and > > TimestampedKeyQuery (and similar for window-header-store; and maybe also > > session-header-store, in case the KIP does include session-header-stores) > > > > > > > > MJS3: Question about naming. You propose to add: > > > > - TimestampedKeyWithHeadersQuery > > - TimestampedRangeWithHeadersQuery > > - WindowKeyWithHeadersQuery > > - WindowRangeWithHeadersQuery > > > > But all four query types return some form of `ValueTimestampHeaders` > > results. So it seems we should align the names, and maybe call the later > > two > > > > - TimestampedWindowKeyWithHeadersQuery > > - TimestampedWindowRangeWithHeadersQuery > > > > Or, to avoid very long names, shorten the first two to > > > > - KeyWithHeadersQuery > > - RangeWithHeadersQuery > > > > > > > > MJS4: About `WindowRangeWithHeadersQuery`. > > > > First some background: The existing `WindowRangeQuery` has two methods > > how to setup the query, and depending which one is used, the query can > > be used against either a windowed-store or a session-store, ie, while > > both are a `WindowRangeQuery`, it's technically two different queries > > for two different stores. > > > > - `withKey(...)` -> only supported by session stores > > - `withWindowStartRange(...)` -> only supported by window store > > > > The KIP proposed to add both methods, too, but at the same time says, > > that window-store won't accept a query created via `withKey` (what in > > general aligns to what we have). But the KIP seems to exclude > > session-header-store support. So why would we add `withKey` at all? > > Would it not be simpler to only add the supported `withWindowStartRange` > > method? > > > > Of course, if we want to mimic the existing query type, the > > WindowRangeWithHeadersQuery seems to face the challenge that > > window-header-store uses `ValueTimestampeHeader` while > > session-header-stores uses `AggregationWithHeaders` type. I would like > > to understand better how this should all fit together. Also for the > > case, that we add session-header-store support later (ie, we should > > avoid to corner ourselves and ensure what we define now, is extensible > > in the future). > > > > > > > > -Matthias > > > > > > > > On 6/11/26 6:23 AM, Jess Jin via dev wrote: > > > Hey Alieh, > > > > > > Thanks for the feedback! Updated in the wiki page. > > > > > > - Jess > > > > > > On Wed, Jun 10, 2026 at 3:26 PM Alieh Saeedi <[email protected]> > > wrote: > > > > > >> Hey Jess, > > >> > > >> Thanks for putting together the KIP. > > >> > > >> A couple of small nits: > > >> > > >> AS1: Please replace the `getX()` methods with `x()`. For example, > > >> `getKey()` should become `key()`. As far as I know, that’s the usual > > >> convention. > > >> AS2: In the `Usage Example` section, please replace `long` with > `Long`, > > >> since `ValueTimestampHeaders.value` is not a primitive. > > >> > > >> - Alieh > > >> > > >> On Wed, Jun 10, 2026 at 7:02 PM Jess Jin via dev < > [email protected]> > > >> wrote: > > >> > > >>> Hi all, > > >>> > > >>> I'd like to start a discussion on KIP-1356: Introduce IQv2 for > > >>> headers-aware state stores. Link > > >>> < > > >>> > > > https://urldefense.com/v3/__https://cwiki.apache.org/confluence/display/KAFKA/KIP-1356*3A*Introduce*IQv2*for*headers-aware*state*stores__;JSsrKysrKw!!Ayb5sqE7!oZFamFyZDrAmJeFHgKc-nOWUWL85bEbfHSdwgrp9jNHVOUCZgoOQ9aiK94L8zPr-tA_zx3WTmNs9vVDlqgQ$ > > >>>> > > >>> > > >>> Thanks. > > >>> > > >> > > > > > > > >
