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