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