Hi, Guozhang, Thanks for the comment!
Hi, Bill, I'll try to make some update to make the KIP better. Thanks for the comment! --- Vito On Sat, Apr 21, 2018 at 5:40 AM, Bill Bejeck <bbej...@gmail.com> wrote: > Hi Vito, > > Thanks for the KIP, overall it's a +1 from me. > > At this point, the only thing I would change is possibly removing the > listing of all methods called by the user and the listing of all store > types and focus on what states result in which exceptions thrown to the > user. > > Thanks, > Bill > > On Fri, Apr 20, 2018 at 2:10 PM, Guozhang Wang <wangg...@gmail.com> wrote: > > > Thanks for the KIP Vito! > > > > I made a pass over the wiki and it looks great to me. I'm +1 on the KIP. > > > > About the base class InvalidStateStoreException itself, I'd actually > > suggest we do not deprecate it but still expose it as part of the public > > API, for people who do not want to handle these cases differently (if we > > deprecate it then we are enforcing them to capture all three exceptions > > one-by-one). > > > > > > Guozhang > > > > > > On Fri, Apr 20, 2018 at 9:14 AM, John Roesler <j...@confluent.io> wrote: > > > > > Hi Vito, > > > > > > Thanks for the KIP! > > > > > > I think it's much nicer to give callers different exceptions to tell > them > > > whether the state store got migrated, whether it's still initializing, > or > > > whether there's some unrecoverable error. > > > > > > In the KIP, it's typically not necessary to discuss non-user-facing > > details > > > such as what exceptions we will throw internally. The KIP is primarily > to > > > discuss public interface changes. > > > > > > You might consider simply removing all the internal details from the > KIP, > > > which will have the dual advantage that it makes the KIP smaller and > > easier > > > to agree on, as well as giving you more freedom in the internal details > > > when it comes to implementation. > > > > > > I like your decision to have your refined exceptions extend > > > InvalidStateStoreException to ensure backward compatibility. Since we > > want > > > to encourage callers to catch the more specific exceptions, and we > don't > > > expect to ever throw a raw InvalidStateStoreException anymore, you > might > > > consider adding the @Deprecated annotation to > InvalidStateStoreException. > > > This will gently encourage callers to migrate to the new exception and > > open > > > the possibility of removing InvalidStateStoreException entirely in a > > future > > > release. > > > > > > Thanks, > > > -John > > > > > > On Thu, Apr 19, 2018 at 8:58 AM, Matthias J. Sax < > matth...@confluent.io> > > > wrote: > > > > > > > Thanks for clarification! That makes sense to me. > > > > > > > > Can you update the KIP to make those suggestions explicit? > > > > > > > > > > > > -Matthias > > > > > > > > On 4/18/18 2:19 PM, vito jeng wrote: > > > > > Matthias, > > > > > > > > > > Thanks for the feedback! > > > > > > > > > >> It's up to you to keep the details part in the KIP or not. > > > > > > > > > > Got it! > > > > > > > > > >> The (incomplete) question was, if we need > `StateStoreFailException` > > or > > > > >> if existing `InvalidStateStoreException` could be used? Do you > > suggest > > > > >> that `InvalidStateStoreException` is not thrown at all anymore, > but > > > only > > > > >> the new sub-classes (just to get a better understanding). > > > > > > > > > > Yes. I suggest that `InvalidStateStoreException` is not thrown at > all > > > > > anymore, > > > > > but only new sub-classes. > > > > > > > > > >> Not sure what this sentence means: > > > > >>> The internal exception will be wrapped as category exception > > finally. > > > > >> Can you elaborate? > > > > > > > > > > For example, `StreamThreadStateStoreProvider#stores()` will throw > > > > > `StreamThreadNotRunningException`(internal exception). > > > > > And then the internal exception will be wrapped as > > > > > `StateStoreRetryableException` or `StateStoreFailException` during > > the > > > > > `KafkaStreams.store()` and throw to the user. > > > > > > > > > > > > > > >> Can you explain the purpose of the "internal exceptions". It's > > unclear > > > > > to me atm why they are introduced. > > > > > > > > > > Hmmm...the purpose of the "internal exceptions" is to distinguish > > > between > > > > > the different kinds of InvalidStateStoreException. > > > > > The original idea is that we can distinguish different state store > > > > > exception for > > > > > different handling. > > > > > But to be honest, I am not quite sure this is necessary. Maybe have > > > > > some change during implementation. > > > > > > > > > > Does it make sense? > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > Vito > > > > > > > > > > On Mon, Apr 16, 2018 at 5:59 PM, Matthias J. Sax < > > > matth...@confluent.io> > > > > > wrote: > > > > > > > > > >> Thanks for the update Vito! > > > > >> > > > > >> It's up to you to keep the details part in the KIP or not. > > > > >> > > > > >> > > > > >> The (incomplete) question was, if we need > `StateStoreFailException` > > or > > > > >> if existing `InvalidStateStoreException` could be used? Do you > > suggest > > > > >> that `InvalidStateStoreException` is not thrown at all anymore, > but > > > only > > > > >> the new sub-classes (just to get a better understanding). > > > > >> > > > > >> > > > > >> Not sure what this sentence means: > > > > >> > > > > >>> The internal exception will be wrapped as category exception > > finally. > > > > >> > > > > >> Can you elaborate? > > > > >> > > > > >> > > > > >> Can you explain the purpose of the "internal exceptions". It's > > unclear > > > > >> to me atm why they are introduced. > > > > >> > > > > >> > > > > >> -Matthias > > > > >> > > > > >> On 4/10/18 12:33 AM, vito jeng wrote: > > > > >>> Matthias, > > > > >>> > > > > >>> Thanks for the review. > > > > >>> I reply separately in the following sections. > > > > >>> > > > > >>> > > > > >>> --- > > > > >>> Vito > > > > >>> > > > > >>> On Sun, Apr 8, 2018 at 1:30 PM, Matthias J. Sax < > > > matth...@confluent.io > > > > > > > > > >>> wrote: > > > > >>> > > > > >>>> Thanks for updating the KIP and sorry for the long pause... > > > > >>>> > > > > >>>> Seems you did a very thorough investigation of the code. It's > > useful > > > > to > > > > >>>> understand what user facing interfaces are affected. > > > > >>> > > > > >>> (Some parts might be even too detailed for a KIP.) > > > > >>>> > > > > >>> > > > > >>> I also think too detailed. Especially the section `Changes in > call > > > > >> trace`. > > > > >>> Do you think it should be removed? > > > > >>> > > > > >>> > > > > >>>> > > > > >>>> To summarize my current understanding of your KIP, the main > change > > > is > > > > to > > > > >>>> introduce new exceptions that extend > `InvalidStateStoreException`. > > > > >>>> > > > > >>> > > > > >>> yep. Keep compatibility in this KIP is important things. > > > > >>> I think the best way is that all new exceptions extend from > > > > >>> `InvalidStateStoreException`. > > > > >>> > > > > >>> > > > > >>>> > > > > >>>> Some questions: > > > > >>>> > > > > >>>> - Why do we need ```? Could `InvalidStateStoreException` be > used > > > for > > > > >>>> this purpose? > > > > >>>> > > > > >>> > > > > >>> Does this question miss some word? > > > > >>> > > > > >>> > > > > >>>> > > > > >>>> - What the superclass of `StateStoreStreamThreadNotRunni > > > ngException` > > > > >>>> is? Should it be `InvalidStateStoreException` or > > > > >> `StateStoreFailException` > > > > >>>> ? > > > > >>>> > > > > >>>> - Is `StateStoreClosed` a fatal or retryable exception ? > > > > >>>> > > > > >>>> > > > > >>> I apologize for not well written parts. I tried to modify some > code > > > in > > > > >> the > > > > >>> recent period and modify the KIP. > > > > >>> The modification is now complete. Please look again. > > > > >>> > > > > >>> > > > > >>>> > > > > >>>> > > > > >>>> -Matthias > > > > >>>> > > > > >>>> > > > > >>>> On 2/21/18 5:15 PM, vito jeng wrote: > > > > >>>>> Matthias, > > > > >>>>> > > > > >>>>> Sorry for not response these days. > > > > >>>>> I just finished it. Please have a look. :) > > > > >>>>> > > > > >>>>> > > > > >>>>> > > > > >>>>> --- > > > > >>>>> Vito > > > > >>>>> > > > > >>>>> On Wed, Feb 14, 2018 at 5:45 AM, Matthias J. Sax < > > > > >> matth...@confluent.io> > > > > >>>>> wrote: > > > > >>>>> > > > > >>>>>> Is there any update on this KIP? > > > > >>>>>> > > > > >>>>>> -Matthias > > > > >>>>>> > > > > >>>>>> On 1/3/18 12:59 AM, vito jeng wrote: > > > > >>>>>>> Matthias, > > > > >>>>>>> > > > > >>>>>>> Thank you for your response. > > > > >>>>>>> > > > > >>>>>>> I think you are right. We need to look at the state both of > > > > >>>>>>> KafkaStreams and StreamThread. > > > > >>>>>>> > > > > >>>>>>> After further understanding of KafkaStreams thread and state > > > store, > > > > >>>>>>> I am currently rewriting the KIP. > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>>> --- > > > > >>>>>>> Vito > > > > >>>>>>> > > > > >>>>>>> On Fri, Dec 29, 2017 at 4:32 AM, Matthias J. Sax < > > > > >>>> matth...@confluent.io> > > > > >>>>>>> wrote: > > > > >>>>>>> > > > > >>>>>>>> Vito, > > > > >>>>>>>> > > > > >>>>>>>> Sorry for this late reply. > > > > >>>>>>>> > > > > >>>>>>>> There can be two cases: > > > > >>>>>>>> > > > > >>>>>>>> - either a store got migrated way and thus, is not hosted > an > > > the > > > > >>>>>>>> application instance anymore, > > > > >>>>>>>> - or, a store is hosted but the instance is in state > > rebalance > > > > >>>>>>>> > > > > >>>>>>>> For the first case, users need to rediscover the store. For > > the > > > > >> second > > > > >>>>>>>> case, they need to wait until rebalance is finished. > > > > >>>>>>>> > > > > >>>>>>>> If KafkaStreams is in state ERROR, PENDING_SHUTDOWN, or > > > > NOT_RUNNING, > > > > >>>>>>>> uses cannot query at all and thus they cannot rediscover or > > > retry. > > > > >>>>>>>> > > > > >>>>>>>> Does this make sense? > > > > >>>>>>>> > > > > >>>>>>>> -Matthias > > > > >>>>>>>> > > > > >>>>>>>> On 12/20/17 12:54 AM, vito jeng wrote: > > > > >>>>>>>>> Matthias, > > > > >>>>>>>>> > > > > >>>>>>>>> I try to clarify some concept. > > > > >>>>>>>>> > > > > >>>>>>>>> When streams state is REBALANCING, it means the user can > just > > > > plain > > > > >>>>>>>> retry. > > > > >>>>>>>>> > > > > >>>>>>>>> When streams state is ERROR or PENDING_SHUTDOWN or > > NOT_RUNNING, > > > > it > > > > >>>>>> means > > > > >>>>>>>>> state store migrated to another instance, the user needs to > > > > >>>> rediscover > > > > >>>>>>>> the > > > > >>>>>>>>> store. > > > > >>>>>>>>> > > > > >>>>>>>>> Is my understanding correct? > > > > >>>>>>>>> > > > > >>>>>>>>> > > > > >>>>>>>>> --- > > > > >>>>>>>>> Vito > > > > >>>>>>>>> > > > > >>>>>>>>> On Sun, Nov 5, 2017 at 12:30 AM, Matthias J. Sax < > > > > >>>>>> matth...@confluent.io> > > > > >>>>>>>>> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>>> Thanks for the KIP Vito! > > > > >>>>>>>>>> > > > > >>>>>>>>>> I agree with what Guozhang said. The original idea of the > > Jira > > > > >> was, > > > > >>>> to > > > > >>>>>>>>>> give different exceptions for different "recovery" > > strategies > > > to > > > > >> the > > > > >>>>>>>> user. > > > > >>>>>>>>>> > > > > >>>>>>>>>> For example, if a store is currently recreated, a user > just > > > need > > > > >> to > > > > >>>>>> wait > > > > >>>>>>>>>> and can query the store later. On the other hand, if a > store > > > go > > > > >>>>>> migrated > > > > >>>>>>>>>> to another instance, a user needs to rediscover the store > > > > instead > > > > >>>> of a > > > > >>>>>>>>>> "plain retry". > > > > >>>>>>>>>> > > > > >>>>>>>>>> Fatal errors might be a third category. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Not sure if there is something else? > > > > >>>>>>>>>> > > > > >>>>>>>>>> Anyway, the KIP should contain a section that talks about > > this > > > > >> ideas > > > > >>>>>> and > > > > >>>>>>>>>> reasoning. > > > > >>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>>> -Matthias > > > > >>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>>> On 11/3/17 11:26 PM, Guozhang Wang wrote: > > > > >>>>>>>>>>> Thanks for writing up the KIP. > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Vito, Matthias: one thing that I wanted to figure out > first > > > is > > > > >> what > > > > >>>>>>>>>>> categories of errors we want to notify the users, if we > > only > > > > >> wants > > > > >>>> to > > > > >>>>>>>>>>> distinguish fatal v.s. retriable then probably we should > > > rename > > > > >> the > > > > >>>>>>>>>>> proposed StateStoreMigratedException / > > > > StateStoreClosedException > > > > >>>>>>>> classes. > > > > >>>>>>>>>>> And then from there we should list what are the possible > > > > internal > > > > >>>>>>>>>>> exceptions ever thrown in those APIs in the call trace, > and > > > > which > > > > >>>>>>>>>>> exceptions should be wrapped to what others, and which > ones > > > > >> should > > > > >>>> be > > > > >>>>>>>>>>> handled without re-throwing, and which ones should not be > > > > wrapped > > > > >>>> at > > > > >>>>>>>> all > > > > >>>>>>>>>>> but directly thrown to user's face. > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Guozhang > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> On Wed, Nov 1, 2017 at 11:09 PM, vito jeng < > > > > v...@is-land.com.tw> > > > > >>>>>>>> wrote: > > > > >>>>>>>>>>> > > > > >>>>>>>>>>>> Hi, > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> I'd like to start discuss KIP-216: > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > >>>>>>>>>>>> 216%3A+IQ+should+throw+different+exceptions+for+ > > > > >> different+errors > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> Please have a look. > > > > >>>>>>>>>>>> Thanks! > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> --- > > > > >>>>>>>>>>>> Vito > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>> > > > > >>>>>>>> > > > > >>>>>>>> > > > > >>>>>>> > > > > >>>>>> > > > > >>>>>> > > > > >>>>> > > > > >>>> > > > > >>>> > > > > >>> > > > > >> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > -- > > -- Guozhang > > >
