Richard, Matthias:

0. Could you describe a bit what are the possible use cases of `allLatest`,
`minKey` and `maxKey`? I'd prefer keeping the APIs to add at a minimum
necessary amount, to avoid a swamp of new APIs that no one would really use
but just complicated the internal code base.

1. One minor comment on the other two new APIs: could we rename `keys` to
`all` and `all` to `range` to be consistent with the other store's APIs?

2. One meta comment on the implementation details: since both `keys` and
`all` would likely touch multiple segments, we may need to use the internal
`SegmentIterator` class, but currently it always requires a Bytes from and
Bytes to for its constructor. What changes we need to make for that class?


Guozhang


On Thu, Oct 5, 2017 at 4:42 PM, Richard Yu <yohan.richard...@gmail.com>
wrote:

> We should split KAFKA-4499 into several sub-issues with 4499 being the
> parent issue.
> Adding the implementation to CachingWindowStore, RocksDBWindowStore, etc
> will each require the addition of a test and implementing the methods which
> is not trivial.
> This way, it should be easier to manage the progress of the KIP.
>
>
> On Thu, Oct 5, 2017 at 2:58 PM, Matthias J. Sax <matth...@confluent.io>
> wrote:
>
> > Thanks for driving this and sorry for late response. With release
> > deadline it was pretty busy lately.
> >
> > Can you please add a description for the suggested method, what they are
> > going to return? It's a little unclear to me atm.
> >
> > It would also be helpful to discuss, for which use case each method is
> > useful. This might also help to identify potential gaps for which
> > another API might be more helpful.
> >
> > Also, we should talk about provided guarantees when using those APIs
> > with regard to consistency -- not saying that we need to provide strong
> > guarantees, but he KIP should describe what user can expect.
> >
> >
> > -Matthias
> >
> > On 9/24/17 8:11 PM, Richard Yu wrote:
> > > Hello, I would like to solicit review and comment on this issue (link
> > > below):
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 205%3A+Add+getAllKeys%28%29+API+to+ReadOnlyWindowStore
> > >
> >
> >
>



-- 
-- Guozhang

Reply via email to