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
