Guozhang, thanks for the comments, I've updated the KIP.
Bill On Wed, Jul 5, 2017 at 10:10 PM, Guozhang Wang <wangg...@gmail.com> wrote: > 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 >
