Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

All, I've updated the KIP and I'd like to start a final round of discussion with an eye towards starting a vote soon, maybe on Monday. Thanks, Bill On Mon, Jun 26, 2017 at 7:26 PM, Guozhang Wang wrote: > Hmm... I'm not sure how this can achieve "closing the store before >

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Hmm... I'm not sure how this can achieve "closing the store before restoration, re-open it with a slight different config, and then close-and-reopen store for query" pattern? You need to be able to access the store object in order to do this right? Guozhang On Mon, Jun 26, 2017 at 7:40 AM, Bill

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Thinking about this some more, I have another approach. Leave the first parameter of as String in the StateRestoreListener interface. But we'll provide 2 default abstract classes one implementing StateRestoreCallback and the other implementing the BatchingStateRestoreCallback. Both abstract

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Guozhang, Thanks for the comments. I think that will work, but my concern is it might not be as clear to users that want to receive external notification of the restore progress separately (say for reporting purposes) and still send separate signals to the state store for resource management

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

More specifically, if we can replace the first parameter from the String store name to the store instance itself, would that be sufficient to cover ` StateRestoreNotification`? On Wed, Jun 21, 2017 at 7:13 PM, Guozhang Wang wrote: > Bill, > > I'm wondering why we need the

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Bill, I'm wondering why we need the `StateRestoreNotification` while still having `StateRestoreListener`, could the above setup achievable just with `StateRestoreListener.onRestoreStart / onRestoreEnd`? I.e. it seems the later can subsume any use cases intended for the former API. Guozhang On

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

I'm going to update the KIP with new interface StateRestoreNotification containing two methods, startRestore and endRestore. While naming is very similar to methods already proposed on the StateRestoreListener, the intent of these methods is not for user notification of restore status. Instead

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Yes it is, more of an oversight on my part, I'll remove it from the KIP. On Mon, Jun 19, 2017 at 12:48 PM, Matthias J. Sax wrote: > Hi, > > I thinks for now it's good enough to start with a single global restore > listener. We can incrementally improve this later on if

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Hi, I thinks for now it's good enough to start with a single global restore listener. We can incrementally improve this later on if required. Of course, if it's easy to do right away we can also be more fine grained. But for KTable, we might want to add this after getting rid of all the overloads

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Thinking about the custom StateRestoreListener approach and having a get method on the interface will really only work for custom state stores. So we'll need to provide another way for users to set behavior with provided state stores. The only option that comes to mind now is also adding a

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Guozhang, Thanks for the comments. 1. As for the granularity, I agree that having one global StateRestoreListener could be restrictive. But I think it's important to have a "setStateRestoreListener" on KafkaStreams as this allows users to define an anonymous instance that has access to local

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Thanks Bill for the updated wiki. I have a couple of more comments: 1. Setting StateRestoreListener on the KafkaStreams granularity may not be sufficient, as in the listener callback we do not which store it is restoring right now: if the topic is a changelog topic then from the `TopicPartition`

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Eno, Thanks for the comments. 1. As for having both restore and restoreAll, I kept the restore method for backward compatibility as that is what is used by current implementing classes. However as I think about it makes things cleaner to have a single restore method taking a collection. I'll

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Thanks Bill, A couple of questions: 1. why do we need both restore and restoreAll, why can't we just have one, that takes a collection (i.e., restore all)? Are there cases when people want to restore one at a time? In that case, they could still use restoreAll with just 1 record in the

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Guozhang, Damian thanks for the comments. Giving developers the ability to hook into StateStore recovery phases was part of my original intent. However the state the KIP is in now won't provide this functionality. As a result I'll be doing a significant revision of KIP-167. I'll be sure to

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

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

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

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

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

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

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

> On Jun 2, 2017, at 12:54 AM, Matthias J. Sax wrote: > > With regard to backward compatibility, we should not change the current > interface, but add a new interface that extends the current one. > ++1

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Thanks for the KIP Bill. With regard to backward compatibility, we should not change the current interface, but add a new interface that extends the current one. If we are going to add "begin" and "after", we might also consider to add some intermediate call backs. This would allow an

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

Sure thing. I'll update the KIP. Thanks, Bill On Thu, Jun 1, 2017 at 6:20 PM, Guozhang Wang wrote: > There are also some request to add "begin" and "after" callbacks in the > restoration func: > > https://issues.apache.org/jira/browse/KAFKA-4322 > > Could we piggy back them

Re: [DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

There are also some request to add "begin" and "after" callbacks in the restoration func: https://issues.apache.org/jira/browse/KAFKA-4322 Could we piggy back them into the same KIP? Guozhang On Thu, Jun 1, 2017 at 2:04 PM, Bill Bejeck wrote: > All, > > I'd like to start

[DISCUSS] KIP-167: Add a restoreAll method to StateRestoreCallback

All, I'd like to start the discussion for adding bulk add functionality when restoring a state store. The KIP can be found here: https://cwiki.apache.org/confluence/display/KAFKA/KIP-167%3A+Add+a+restoreAll+method+to+StateRestoreCallback Thanks, Bill