Matthias, Thank you for your assistance.
> what is the status of this KIP? Unfortunately, there is no further progress. About seven weeks ago, I was injured in sports. I had a broken wrist on my left wrist. Many jobs are affected, including this KIP and implementation. > I just re-read it, and have a couple of follow up comments. Why do we > discuss the internal exceptions you want to add? Also, do we really need > them? Can't we just throw the correct exception directly instead of > wrapping it later? I think you may be right. As I say in the previous: "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." During the implementation, I also feel we maybe not need wrapper it. We can just throw the correctly directly. > Is `StreamThreadNotRunningException` really an retryable error? When KafkaStream state is REBALANCING, I think it is a retryable error. StreamThreadStateStoreProvider#stores() will throw StreamThreadNotRunningException when StreamThread state is not RUNNING. The user can retry until KafkaStream state is RUNNING. > When would we throw an `StateStoreEmptyException`? The semantics is unclear to me atm. > When the state is RUNNING, is `StateStoreClosedException` a retryable error? These two comments will be answered in another mail. --- Vito On Mon, Jun 11, 2018 at 8:12 AM, Matthias J. Sax <matth...@confluent.io> wrote: > Vito, > > what is the status of this KIP? > > I just re-read it, and have a couple of follow up comments. Why do we > discuss the internal exceptions you want to add? Also, do we really need > them? Can't we just throw the correct exception directly instead of > wrapping it later? > > When would we throw an `StateStoreEmptyException`? The semantics is > unclear to me atm. > > Is `StreamThreadNotRunningException` really an retryable error? > > When the state is RUNNING, is `StateStoreClosedException` a retryable > error? > > One more nits: ReadOnlyWindowStore got a new method #fetch(K key, long > time); that should be added > > > Overall I like the KIP but some details are still unclear. Maybe it > might help if you open an PR in parallel? > > > -Matthias > > On 4/24/18 8:18 AM, vito jeng wrote: > > 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 > >>> > >> > > > >
