Hey Vito, I saw that you updated your PR, but did not reply to my last comments. Any thoughts?
-Matthias On 10/19/18 10:34 AM, Matthias J. Sax wrote: > Glad to have you back Vito :) > > Some follow up thoughts: > > - the current `InvalidStateStoreException` is documents as being > sometimes retry-able. From the JavaDocs: > >> These exceptions may be transient [...] Hence, it is valid to backoff and >> retry when handling this exception. > > I am wondering what the semantic impact/change is, if we introduce > `RetryableStateStoreException` and `FatalStateStoreException` that both > inherit from it. While I like the introduction of both from a high level > point of view, I just want to make sure it's semantically sound and > backward compatible. Atm, I think it's fine, but I want to point it out > such that everybody can think about this, too, so we can verify that > it's a natural evolving API change. > > - StateStoreClosedException: > >> will be wrapped to StateStoreMigratedException or >> StateStoreNotAvailableException later. > > Can you clarify the cases (ie, when will it be wrapped with the one or > the other)? > > - StateStoreIsEmptyException: > > I don't understand the semantic of this exception. Maybe it's a naming > issue? > >> will be wrapped to StateStoreMigratedException or >> StateStoreNotAvailableException later. > > Also, can you clarify the cases (ie, when will it be wrapped with the > one or the other)? > > > I am also wondering, if we should introduce a fatal exception > `UnkownStateStoreException` to tell users that they passed in an unknown > store name? > > > > -Matthias > > > > On 10/17/18 8:14 PM, vito jeng wrote: >> Just open a PR for further discussion: >> https://github.com/apache/kafka/pull/5814 >> >> Any suggestion is welcome. >> Thanks! >> >> --- >> Vito >> >> >> On Thu, Oct 11, 2018 at 12:14 AM vito jeng <v...@is-land.com.tw> wrote: >> >>> Hi John, >>> >>> Thanks for reviewing the KIP. >>> >>>> I didn't follow the addition of a new method to the QueryableStoreType >>>> interface. Can you elaborate why this is necessary to support the new >>>> exception types? >>> >>> To support the new exception types, I would check stream state in the >>> following classes: >>> - CompositeReadOnlyKeyValueStore class >>> - CompositeReadOnlySessionStore class >>> - CompositeReadOnlyWindowStore class >>> - DelegatingPeekingKeyValueIterator class >>> >>> It is also necessary to keep backward compatibility. So I plan passing >>> stream >>> instance to QueryableStoreType instance during KafkaStreams#store() >>> invoked. >>> It looks a most simple way, I think. >>> >>> It is why I add a new method to the QueryableStoreType interface. I can >>> understand >>> that we should try to avoid adding the public api method. However, at the >>> moment >>> I have no better ideas. >>> >>> Any thoughts? >>> >>> >>>> Also, looking over your KIP again, it seems valuable to introduce >>>> "retriable store exception" and "fatal store exception" marker interfaces >>>> that the various exceptions can mix in. It would be nice from a usability >>>> perspective to be able to just log and retry on any "retriable" exception >>>> and log and shutdown on any fatal exception. >>> >>> I agree that this is valuable to the user. >>> I'll update the KIP. >>> >>> >>> Thanks >>> >>> >>> --- >>> Vito >>> >>> >>> On Tue, Oct 9, 2018 at 2:30 AM John Roesler <j...@confluent.io> wrote: >>> >>>> Hi Vito, >>>> >>>> I'm glad to hear you're well again! >>>> >>>> I didn't follow the addition of a new method to the QueryableStoreType >>>> interface. Can you elaborate why this is necessary to support the new >>>> exception types? >>>> >>>> Also, looking over your KIP again, it seems valuable to introduce >>>> "retriable store exception" and "fatal store exception" marker interfaces >>>> that the various exceptions can mix in. It would be nice from a usability >>>> perspective to be able to just log and retry on any "retriable" exception >>>> and log and shutdown on any fatal exception. >>>> >>>> Thanks, >>>> -John >>>> >>>> On Fri, Oct 5, 2018 at 11:47 AM Guozhang Wang <wangg...@gmail.com> wrote: >>>> >>>>> Thanks for the explanation, that makes sense. >>>>> >>>>> >>>>> Guozhang >>>>> >>>>> >>>>> On Mon, Jun 25, 2018 at 2:28 PM, Matthias J. Sax <matth...@confluent.io >>>>> >>>>> wrote: >>>>> >>>>>> The scenario I had I mind was, that KS is started in one thread while >>>> a >>>>>> second thread has a reference to the object to issue queries. >>>>>> >>>>>> If a query is issue before the "main thread" started KS, and the >>>> "query >>>>>> thread" knows that it will eventually get started, it can retry. On >>>> the >>>>>> other hand, if KS is in state PENDING_SHUTDOWN or DEAD, it is >>>> impossible >>>>>> to issue any query against it now or in the future and thus the error >>>> is >>>>>> not retryable. >>>>>> >>>>>> >>>>>> -Matthias >>>>>> >>>>>> On 6/25/18 10:15 AM, Guozhang Wang wrote: >>>>>>> I'm wondering if StreamThreadNotStarted could be merged into >>>>>>> StreamThreadNotRunning, because I think users' handling logic for >>>> the >>>>>> third >>>>>>> case would be likely the same as the second. Do you have some >>>> scenarios >>>>>>> where users may want to handle them differently? >>>>>>> >>>>>>> Guozhang >>>>>>> >>>>>>> On Sun, Jun 24, 2018 at 5:25 PM, Matthias J. Sax < >>>>> matth...@confluent.io> >>>>>>> wrote: >>>>>>> >>>>>>>> Sorry to hear! Get well soon! >>>>>>>> >>>>>>>> It's not a big deal if the KIP stalls a little bit. Feel free to >>>> pick >>>>> it >>>>>>>> up again when you find time. >>>>>>>> >>>>>>>>>>> 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. >>>>>>>> >>>>>>>> I see. If this is the intention, than I would suggest to have two >>>> (or >>>>>>>> maybe three) different exceptions: >>>>>>>> >>>>>>>> - StreamThreadRebalancingException (retryable) >>>>>>>> - StreamThreadNotRunning (not retryable -- thrown if in state >>>>>>>> PENDING_SHUTDOWN or DEAD >>>>>>>> - maybe StreamThreadNotStarted (for state CREATED) >>>>>>>> >>>>>>>> The last one is tricky and could also be merged into one of the >>>> first >>>>>>>> two, depending if you want to argue that it's retryable or not. >>>> (Just >>>>>>>> food for though -- not sure what others think.) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -Matthias >>>>>>>> >>>>>>>> On 6/22/18 8:06 AM, vito jeng wrote: >>>>>>>>> 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 >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> -- Guozhang >>>>> >>>> >>> >> >
signature.asc
Description: OpenPGP digital signature
