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 >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
