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
