Thanks John!
PS. vote thread started
https://lists.apache.org/thread.html/r5863299712063d4cb4be139ce4ab0533ce66efe04c1f7e993666b09c%40%3Cdev.kafka.apache.org%3E
On Sat, Sep 19, 2020 at 2:46 AM John Roesler <vvcep...@apache.org> wrote:
> 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.
> > > >
> > >
> >
>
