Hi, Matthias, This is second part.
> For the internal exceptions: > > `StateStoreClosedException` -- why can it be wrapped as > `StreamThreadNotStartedException` ? It seems that the later would only > be thrown by `KafkaStreams#store()` and thus would be throw directly. Both `StateStoreClosedException` and `EmptyStateStoreException` not can be wrapped as `StreamThreadNotStartedException`. This is a mistaken written in the previous KIP. Thank you point this. > A closed-exception should only happen after a store was successfully > retrieved but cannot be queried any longer? Hence, converting/wrapping > it into a `StateStoreMigratedException` make sense. I am also not sure, > when a closed-exception would be wrapped by a > `StateStoreNotAvailableException` (implying my understanding as describe > above)? > > Same questions about `EmptyStateStoreException`. > > Thinking about both internal exceptions twice, I am wondering if it > makes sense to have both internal exceptions at all? I have the > impression that it make only sense to wrap them with a > `StateStoreMigragedException`, but if they are wrapped into the same > exception all the time, we can just remove both and throw > `StateStoreMigratedException` directly? After deeper thinking, I think you are right. It seems we can throw `StateStoreMigratedException` directly. So that we can remove `StateStoreClosedException`, `EmptyStateStoreException` and `StateStoreNotAvailableException`. Will update the KIP. BTW, if we remove above three exceptions, the `StreamThreadNotRunningException` will be the only one sub class extends from FatalStateStoreException. Should we remove `StreamThreadNotRunningException` and throw `FatalStateStoreException` directly ? > Last point: Why do we need to add? > QueryableStoreType#setStreams(KafkaStreams streams); > John asked this question already and you replied to it. But I am not > sure what your answer means. Can you explain it in more detail? The main purpose is to pass the KafkaStreams reference into CompositeReadOnlyKeyValueStore / CompositeReadOnlySessionStore/ CompositeReadOnlyWindowStore instance. We need check KafkaStreams state to warp InvalidStateStoreException in to other exception(e.g., StateStoreMigratedException) when the user accesses these read-only stores. The original thought is to add `setStreams` method in to QueryableStoreType. But now I think I find a better way during recent days. This way does not need to change any public interface. So we can skip this question. :) I will update the KIP based on our discussion. Thank you for help to finish the KIP! --- Vito On Thu, Jun 6, 2019 at 8:23 AM Matthias J. Sax <matth...@confluent.io> wrote: > Hi Vito, > > sorry for dropping this discussion on the floor a while back. I was just > re-reading the KIP and discussion thread, and I think it is shaping out > nicely! > > I like the overall hierarchy of the exception classes. > > Some things are still not 100% clear: > > > You listed all methods that may throw an `InvalidStateStoreException` > atm. For the new exceptions, can any exception be thrown by any method? > It might help to understand this relationship better. > > For example, StreamThreadNotStartedException, seems to only make sense > for `KafkaStreams#store()`? > > > For `StreamThreadNotRunningException` should we rename it to > `KafkaStreamsNotRunningException` ? > > > The description of `StreamThreadNotRunningException` and > `StateStoreNotAvailableException` seems to be the same? From my > understandng, the description makes sense for > `StreamThreadNotRunningException` -- for > `StateStoreNotAvailableException` I was expecting/inferring from the > name, that it would be thrown if no such store exists in the topology at > all (ie, user passed in a invalid/wrong store name). For this case, this > exception should be thrown only from `KafkaStreams#store()` ? > > > For the internal exceptions: > > `StateStoreClosedException` -- why can it be wrapped as > `StreamThreadNotStartedException` ? It seems that the later would only > be thrown by `KafkaStreams#store()` and thus would be throw directly. A > closed-exception should only happen after a store was successfully > retrieved but cannot be queried any longer? Hence, converting/wrapping > it into a `StateStoreMigratedException` make sense. I am also not sure, > when a closed-exception would be wrapped by a > `StateStoreNotAvailableException` (implying my understanding as describe > above)? > > Same questions about `EmptyStateStoreException`. > > Thinking about both internal exceptions twice, I am wondering if it > makes sense to have both internal exceptions at all? I have the > impression that it make only sense to wrap them with a > `StateStoreMigragedException`, but if they are wrapped into the same > exception all the time, we can just remove both and throw > `StateStoreMigratedException` directly? > > > Last point: Why do we need to add? > > > QueryableStoreType#setStreams(KafkaStreams streams); > > John asked this question already and you replied to it. But I am not > sure what your answer means. Can you explain it in more detail? > > > > Thanks for your patience on this KIP! > > > > -Matthias > > > > > > > On 11/11/18 4:55 AM, Vito Jeng wrote: > > Hi, Matthias, > > > > KIP already updated. > > > >> - 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)? > > > > For example, in the implementation(CompositeReadOnlyKeyValueStore#get), > we > > get all stores first, and then call ReadOnlyKeyValueStore#get to get > value > > in every store iteration. > > > > When calling ReadOnlyKeyValueStore#get, the StateStoreClosedException > will > > be thrown if the state store is not open. > > We need catch StateStoreClosedException and wrap it in different > exception > > type: > > * If the stream's state is CREATED, we wrap StateStoreClosedException > > with StreamThreadNotStartedException. User can retry until to RUNNING. > > * If the stream's state is RUNNING / REBALANCING, the state store > should > > be migrated, we wrap StateStoreClosedException with > > StateStoreMigratedException. User can rediscover the state store. > > * If the stream's state is PENDING_SHUTDOWN / NOT_RUNNING / ERROR, the > > stream thread is not available, we wrap StateStoreClosedException with > > StateStoreNotAvailableException. User cannot retry when this exception is > > thrown. > > > > > >> - StateStoreIsEmptyException: > >> I don't understand the semantic of this exception. Maybe it's a naming > > issue? > > > > I think yes. :) > > Does `EmptyStateStoreException` is better ? (already updated in the KIP) > > > > > >> - StateStoreIsEmptyException: > >> 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)? > > > > For example, in the implementation (CompositeReadOnlyKeyValueStore#get), > we > > call StateStoreProvider#stores (WrappingStoreProvider#stores) to get all > > stores. EmptyStateStoreException will be thrown when cannot find any > store > > and then we need catch it and wrap it in different exception type: > > * If the stream's state is CREATED, we wrap EmptyStateStoreException > with > > StreamThreadNotStartedException. User can retry until to RUNNING. > > * If the stream's state is RUNNING / REBALANCING, the state store > should > > be migrated, we wrap EmptyStateStoreException with > > StateStoreMigratedException. User can rediscover the state store. > > * If the stream's state is PENDING_SHUTDOWN / NOT_RUNNING / ERROR, the > > stream thread is not available, we wrap EmptyStateStoreException with > > StateStoreNotAvailableException. User cannot retry when this exception is > > thrown. > > > > I hope the above reply can clarify. > > > > The last one that was not replied was: > > > >> I am also wondering, if we should introduce a fatal exception > >> `UnkownStateStoreException` to tell users that they passed in an unknown > >> store name? > > > > Until now, unknown state store is not thinking about in the KIP. > > I believe it would be very useful for users. > > > > Looking at the related code(WrappingStoreProvider#stores), > > I found that I can't distinguish between the state store was migrated or > an > > unknown state store. > > > > Any thoughts? > > > > --- > > Vito > > > > > > > > On Sun, Nov 11, 2018 at 5:31 PM Vito Jeng <v...@is-land.com.tw> wrote: > > > >> 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 > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > > > >
