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