Hi, Matthias, Sorry for the late reply.
> 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. Thank you for pointing this out. This's really important for public API. Just when I was replying to you, I found that KIP needs some modify. I will fix it ASAP, and then let's continue the discussion. --- Vito On Wed, Nov 7, 2018 at 7:06 AM Matthias J. Sax <matth...@confluent.io> wrote: > 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 > >>>>> > >>>> > >>> > >> > > > >
