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 > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > > >
