I'm largely in agreement with what Guozhang has suggested, i.e.,
StateRestoreContext shouldn't have any setters on it and also need to have
the end offset available such that people can use it derive progress.
Slightly different, maybe the StateRestoreContext interface could be:
long beginOffset()
long endOffset()
long currentOffset()
One further thing, this currently doesn't provide developers the ability to
hook into this information if they are using the built-in StateStores. Is
this something we should be considering?
On Tue, 6 Jun 2017 at 23:32 Guozhang Wang <wangg...@gmail.com> wrote:
> Thanks for the updated KIP Bill, I have a couple of comments:
>
> 1) I'm assuming beginRestore / endRestore is called only once per store
> throughout the whole restoration process, and restoreAll is called per
> batch. In that case I feel we can set the StateRestoreContext as a second
> parameter in restoreAll and in endRestore as well, and let the library to
> set the corresponding values instead and only let users to read (since the
> collection of key-value pairs do not contain offset information anyways
> users cannot really set the offset). The "lastOffsetRestored" would be the
> starting offset when called on `beginRestore`.
>
> 2) Users who wants to implement their own batch restoration callbacks would
> now need to implement both `restore` and `restoreAll` while they either let
> `restoreAll` to call `restore` or implement the logic in `restoreAll` only
> and never call `restore`. Maybe we can provide two abstract impl of
> BatchingStateRestoreCallbacks which does beginRestore / endRestore as
> no-ops, with one callback implementing `restoreAll` to call abstract
> `restore` while the other implement `restore` to throw "not supported
> exception" and keep `restoreAll` abstract.
>
> 3) I think we can also return the "offset limit" in StateRestoreContext,
> which is important for users to track the restoration progress since
> otherwise they could not tell how many percent of restoration has
> completed. I.e.:
>
> public interface BatchingStateRestoreCallback extends StateRestoreCallback
> {
>
> void restoreAll(Collection<KeyValue<byte[], byte []>> records,
> StateRestoreContext
> restoreContext);
>
> void beginRestore(StateRestoreContext restoreContext);
>
> void endRestore(StateRestoreContext restoreContext);
> }
>
> public interface StateRestoreContext {
>
> long lastOffsetRestored();
>
> long endOffsetToRestore();
>
> int numberRestored();
> }
>
>
> Guozhang
>
>
>
> On Fri, Jun 2, 2017 at 9:16 AM, Bill Bejeck <bbej...@gmail.com> wrote:
>
> > Guozhang, Matthias,
> >
> > Thanks for the comments. I have updated the KIP, (JIRA title and
> > description as well).
> >
> > I had thought about introducing a separate interface altogether, but
> > extending the current one makes more sense.
> >
> > As for intermediate callbacks based on time or number of records, I think
> > the latest update to the KIP addresses this point of querying for
> > intermediate results, but it would be per batch restored.
> >
> > Thanks,
> > Bill
> >
> >
> >
> >
> >
> > On Fri, Jun 2, 2017 at 8:36 AM, Jim Jagielski <j...@jagunet.com> wrote:
> >
> > >
> > > > On Jun 2, 2017, at 12:54 AM, Matthias J. Sax <matth...@confluent.io>
> > > wrote:
> > > >
> > > > With regard to backward compatibility, we should not change the
> current
> > > > interface, but add a new interface that extends the current one.
> > > >
> > >
> > > ++1
> > >
> > >
> >
>
>
>
> --
> -- Guozhang
>
