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
