Thanks for the KIP, Jorge!
Sorry it took me so long. I just reviewed the KIP, and it looks good to me.
Thanks,
John
On Tue, Sep 1, 2020, at 12:35, Jorge Esteban Quilcate Otoya wrote:
> Thanks Sophie!
>
> > one nit: you missed updating the startTime long to Instant in both
> appearances of the fetchSession(key, startTime, sessionEndTime) method.
>
> Agreed. I'm fixing this on the KIP.
>
> > Also, I think by "startTime" you actually meant "earliestSessionEndTime".
>
> Given the implementation usage of these variables, I think it refers to
> session start and end time, e.g. in `InMemorySessionStore`:
>
> ```
> public byte[] fetchSession(final Bytes key, final long startTime, final
> long endTime) {
> removeExpiredSegments();
>
> Objects.requireNonNull(key, "key cannot be null");
>
> // Only need to search if the record hasn't expired yet
> if (endTime > observedStreamTime - retentionPeriod) {
> final ConcurrentNavigableMap<Bytes,
> ConcurrentNavigableMap<Long, byte[]>> keyMap = endTimeMap.get(endTime);
> if (keyMap != null) {
> final ConcurrentNavigableMap<Long, byte[]> startTimeMap =
> keyMap.get(key);
> if (startTimeMap != null) {
> return startTimeMap.get(startTime);
> }
> }
> }
> return null;
> }
> ```
>
> > One question I do have is whether we really need to provide a default
> implementation
> > that throws UnsupportedOperationException? Actually I'm wondering if we
> shouldn't
> > do something similar to the WindowStore methods, and provide a default
> implementation
> > on the SessionStore interface which then just calls the corresponding
> long-based method.
>
> I'll be happy with this approach. I was trying to align it with the
> approach we followed on KIP-617, but you're right in the case of
> WindowStore we didn't add default implementations. I'll update the KIP
> accordingly and wait for more feedback.
>
> Cheers,
> Jorge.
>
>
>
> On Tue, Sep 1, 2020 at 4:51 AM Sophie Blee-Goldman <sop...@confluent.io>
> wrote:
>
> > Thanks for bringing the IQ API into alignment -- the proposal looks good,
> > although
> > one nit: you missed updating the startTime long to Instant in both
> > appearances of
> > the fetchSession(key, startTime, sessionEndTime) method. Also, I think by
> > "startTime"
> > you actually meant "earliestSessionEndTime".
> >
> > One question I do have is whether we really need to provide a default
> > implementation
> > that throws UnsupportedOperationException? Actually I'm wondering if we
> > shouldn't
> > do something similar to the WindowStore methods, and provide a default
> > implementation
> > on the SessionStore interface which then just calls the corresponding
> > long-based method.
> > WDYT?
> >
> > -Sophie
> >
> > On Fri, Aug 28, 2020 at 11:31 AM Jorge Esteban Quilcate Otoya <
> > quilcate.jo...@gmail.com> wrote:
> >
> > > Hi everyone,
> > >
> > > I'd like to discuss the following proposal to align IQ Session Store API
> > > with the Window Store one.
> > >
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-666%3A+Add+Instant-based+methods+to+ReadOnlySessionStore
> > >
> > > Looking forward to your feedback.
> > >
> > > Cheers,
> > > Jorge.
> > >
> >
>
