Hi, Mattias, Just completed the modification of KIP, please take a look when you are available.
--- Vito On Wed, Jul 3, 2019 at 9:07 PM Vito Jeng <v...@is-land.com.tw> wrote: > 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 >> >>>>>>>> >> >>>>>>> >> >>>>>> >> >>>>> >> >>>> >> >>> >> >>> >> > >> >>
