Hi, Matthias, Thanks for the review!
I have updated the KIP, please look again. Thanks. --- Vito On Thu, Apr 19, 2018 at 9:58 PM, 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 `StateStoreStreamThreadNotRunningException` > >>>> 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 > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > > >
