Bill, Thanks for the updated KIP. From your WIP PR I think one piece of information that still need to be included in the KIP is that the library will check the type of the restoreCallback in runtime, and if it is extending StateRestoreListener it will also execute its functions at the runtime. SO StateRestoreListener's APIs will be called in two places:
1) if the instance-level listener is set, then triggered on each poll call; 2) if the state-store-level callback is extending the listener, then triggered for each poll call that is restoring that specific store. Otherwise it may be still confusing to readers. Guozhang On Fri, Jun 30, 2017 at 2:08 PM, Bill Bejeck <bbej...@gmail.com> wrote: > Hi Eno, > > Thanks for the comment. > > Depending on how users implement the `onBatchRestore` it could potentially > slow down the restoration. > > However IMHO it shouldn't present an issue as in practice I suspect users > simply want a status update of how far along the restoration process has > progressed. > > But having said that, other than documenting users need to keep processing > in `onBatchRestore` to an absolute minimum I don't have any good ideas on > how to mitigate that situation ATM (making the call to `onBatchRestore` in > an async manner? but adds complexity). WDYT? > > Thanks, > Bill > > On Fri, Jun 30, 2017 at 12:37 PM, Eno Thereska <eno.there...@gmail.com> > wrote: > > > Thanks Bill, > > > > My only remaining question is whether we expect that calling > > `onBatchRestore` after each `poll` could slow down the restoration? > > > > Thanks > > Eno > > > > > > > On Jun 30, 2017, at 4:27 PM, Bill Bejeck <bbej...@gmail.com> wrote: > > > > > > Damian, > > > > > > Thanks for comments. I've updated the KIP with the abstract classes. > > > > > > Thanks, > > > Bill > > > > > > On Fri, Jun 30, 2017 at 9:57 AM, Damian Guy <damian....@gmail.com> > > wrote: > > > > > >> Thanks for the updated KIP Bill. > > >> > > >> In the PR you have AbstractBatchingRestoreCallback and > > >> AbstractNotifyingRestoreCallback which are both in public packages, > so > > are > > >> part of the API. I think you need to add those to the KIP to round it > > off. > > >> > > >> Otherwise LGTM. > > >> > > >> Thanks, > > >> Damian > > >> > > >> On Fri, 30 Jun 2017 at 12:39 Bill Bejeck <bbej...@gmail.com> wrote: > > >> > > >>> Hi, > > >>> > > >>> I updated the KIP yesterday and reposted on the original thread, but > I > > >>> think it may get lost in the shuffle. I'd like to have one more > round > > of > > >>> discussion on the KIP found here: > > >>> > > >>> > > >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >> 167%3A+Add+interface+for+the+state+store+restoration+process > > >>> > > >>> There's also an initial PR here : > > >>> https://github.com/apache/kafka/pull/3325 > > >>> > > >> > > > > > -- -- Guozhang
