Got it, thanks Matthias. --- Vito
On Thu, Jan 23, 2020 at 1:31 PM Matthias J. Sax <matth...@confluent.io> wrote: > Thanks Vito. > > I am also ok with either name. Just a personal slight preference, but > not a important. > > > -Matthias > > On 1/21/20 6:52 PM, Vito Jeng wrote: > > Thanks Matthias. > > > > The KIP is about InvalidStateStoreException. > > I pick `StateStoreNotAvailableException` because it may be more intuitive > > than `StreamsNotRunningException`. > > > > No matter which one picked, it's good to me. > > > > --- > > Vito > > > > > > On Wed, Jan 22, 2020 at 7:44 AM Matthias J. Sax <matth...@confluent.io> > > wrote: > > > >> Thanks for updating the KIP! > >> > >> One last comment/question: you kept `StateStoreNotAvailableException` in > >> favor of `StreamsNotRunningException` (to merge both as suggested). > >> > >> I am wondering, if it might be better to keep > >> `StreamsNotRunningException` instead of > >> `StateStoreNotAvailableException`, because this exception is thrown if > >> Streams is in state PENDING_SHUTDOWN / NOT_RUNNING / ERROR ? > >> > >> > >> > >> -Matthias > >> > >> On 1/17/20 9:56 PM, John Roesler wrote: > >>> Thanks, Vito. I've just cast my vote. > >>> -John > >>> > >>> On Fri, Jan 17, 2020, at 21:32, Vito Jeng wrote: > >>>> Hi, folks, > >>>> > >>>> Just update the KIP, please take a look. > >>>> > >>>> Thanks! > >>>> > >>>> --- > >>>> Vito > >>>> > >>>> > >>>> On Fri, Jan 17, 2020 at 9:12 AM Vito Jeng <v...@is-land.com.tw> > wrote: > >>>> > >>>>> Thanks Bill, John and Matthias. Glad you guys joined this discussion. > >>>>> I got a lot out of the discussion. > >>>>> > >>>>> I would like to update KIP-216 base on John's suggestion to remove > the > >>>>> category. > >>>>> > >>>>> > >>>>> --- > >>>>> Vito > >>>>> > >>>>> > >>>>> On Fri, Jan 17, 2020 at 2:30 AM Matthias J. Sax < > matth...@confluent.io > >>> > >>>>> wrote: > >>>>> > >>>>>>> Nevertheless, if we omit the categorization, it’s moot. > >>>>>> > >>>>>> Ack. > >>>>>> > >>>>>> I am fine to remove the middle tier. As John pointed out, it might > be > >>>>>> weird to have only one concrete exception type per category. We can > >> also > >>>>>> explain in detail how to handle each exception in their JavaDocs. > >>>>>> > >>>>>> > >>>>>> -Matthias > >>>>>> > >>>>>> On 1/16/20 6:38 AM, Bill Bejeck wrote: > >>>>>>> Vito, > >>>>>>> > >>>>>>> Thanks for the updates, the KIP LGTM. > >>>>>>> > >>>>>>> -Bill > >>>>>>> > >>>>>>> On Wed, Jan 15, 2020 at 11:31 PM John Roesler <vvcep...@apache.org > > > >>>>>> wrote: > >>>>>>> > >>>>>>>> Hi Vito, > >>>>>>>> > >>>>>>>> Haha, your archive game is on point! > >>>>>>>> > >>>>>>>> What Matthias said in that email is essentially what I figured was > >> the > >>>>>>>> rationale. It makes sense, but the point I was making is that this > >>>>>> really > >>>>>>>> doesn’t seem like a good way to structure a production app. On the > >>>>>> other > >>>>>>>> hand, considering the exception fatal has a good chance of > avoiding > >> a > >>>>>>>> frustrating debug session if you just forgot to call start. > >>>>>>>> > >>>>>>>> Nevertheless, if we omit the categorization, it’s moot. > >>>>>>>> > >>>>>>>> It would be easy to add a categorization layer later if we want > it, > >> but > >>>>>>>> not very easy to change it if we get it wrong. > >>>>>>>> > >>>>>>>> Thanks for your consideration! > >>>>>>>> -John > >>>>>>>> > >>>>>>>> On Wed, Jan 15, 2020, at 21:14, Vito Jeng wrote: > >>>>>>>>> Hi John, > >>>>>>>>> > >>>>>>>>> About `StreamsNotStartedException is strange` -- > >>>>>>>>> The original idea came from Matthias, two years ago. :) > >>>>>>>>> You can reference here: > >>>>>>>>> > >>>>>>>> > >>>>>> > >> > https://mail-archives.apache.org/mod_mbox/kafka-dev/201806.mbox/%3c6c32083e-b63c-435b-521d-032d45cc5...@confluent.io%3e > >>>>>>>>> > >>>>>>>>> About omitting the categorization -- > >>>>>>>>> It looks reasonable. I'm fine with omitting the categorization > but > >> not > >>>>>>>> very > >>>>>>>>> sure it is a good choice. > >>>>>>>>> Does any other folks provide opinion? > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> Hi, folks, > >>>>>>>>> > >>>>>>>>> Just update the KIP-216, please take a look. > >>>>>>>>> > >>>>>>>>> --- > >>>>>>>>> Vito > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Thu, Jan 16, 2020 at 6:35 AM Vito Jeng <v...@is-land.com.tw> > >>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Hi, folks, > >>>>>>>>>> > >>>>>>>>>> Thank you suggestion, really appreciate it. :) > >>>>>>>>>> I understand your concern. I'll merge StreamsNotRunningException > >> and > >>>>>>>>>> StateStoreNotAvailableException. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> --- > >>>>>>>>>> Vito > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On Thu, Jan 16, 2020 at 6:22 AM John Roesler < > vvcep...@apache.org > >>> > >>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>>> Hey Vito, > >>>>>>>>>>> > >>>>>>>>>>> Yes, thanks for the KIP. Sorry the discussion has been so long. > >>>>>>>>>>> Hopefully, we can close it out soon. > >>>>>>>>>>> > >>>>>>>>>>> I agree we can drop StreamsNotRunningException in favor of > >>>>>>>>>>> just StateStoreNotAvailableException. > >>>>>>>>>>> > >>>>>>>>>>> Unfortunately, I have some higher-level concerns. The value > >>>>>>>>>>> of these exceptions is that they tell you how to handle the > >>>>>>>>>>> various situations that can arise while querying a distributed > >>>>>>>>>>> data store. > >>>>>>>>>>> > >>>>>>>>>>> Ideally, as a caller, I should be able to just catch > "retriable" > >> or > >>>>>>>>>>> "fatal" and handle them appropriately. Otherwise, there's no > >>>>>>>>>>> point in having categories, and we should just have all the > >>>>>>>>>>> exceptions extend InvalidStateStoreException. > >>>>>>>>>>> > >>>>>>>>>>> Presently, it's not possible to tell from just the > >>>>>>>>>>> "retriable"/"fatal" distinction what to do. You can tell > >>>>>>>>>>> from the descriptions of the various exceptions. E.g.: > >>>>>>>>>>> > >>>>>>>>>>> Retriable: > >>>>>>>>>>> * StreamsRebalancingException: the exact same call > >>>>>>>>>>> should just be retried until the rebalance is complete > >>>>>>>>>>> * StateStoreMigratedException: the store handle is > >>>>>>>>>>> now invalid, so you need to re-discover the instance > >>>>>>>>>>> and get a new handle on that instance. In other words, > >>>>>>>>>>> the query itself may be valid, but the particular method > >>>>>>>>>>> invocation on this particular instance has encountered > >>>>>>>>>>> a fatal exception. > >>>>>>>>>>> > >>>>>>>>>>> Fatal: > >>>>>>>>>>> * UnknownStateStoreException: this is truly fatal. No amount > >>>>>>>>>>> of retrying or re-discovering is going to get you a handle > >> on a > >>>>>>>>>>> store that doesn't exist in the cluster. > >>>>>>>>>>> * StateStoreNotAvailableException: this is actually > recoverable, > >>>>>>>>>>> since the store might exist in the cluster, but isn't > >> available > >>>>>> on > >>>>>>>>>>> this particular instance (which is shut down or whatever). > >>>>>>>>>>> > >>>>>>>>>>> Personally, I'm not a fan of code bureaucracy, so I'm 100% fine > >>>>>>>>>>> with omitting the categorization and just having 5 subclasses > >>>>>>>>>>> of InvalidStateStoreException. Each of them would tell you > >>>>>>>>>>> how to handle them, and it's not too many to really > >>>>>>>>>>> understand and handle each one. > >>>>>>>>>>> > >>>>>>>>>>> If you really want to have a middle tier, I'd recommend: > >>>>>>>>>>> * RetryableStateStoreException: the exact same call > >>>>>>>>>>> should be repeated. > >>>>>>>>>>> * RecoverableStateStoreException: the store handle > >>>>>>>>>>> should be discarded and the caller should re-discover > >>>>>>>>>>> the location of the store and repeat the query on the > >>>>>>>>>>> correct instance. > >>>>>>>>>>> * FatalStateStoreException: the query/request is totally > >>>>>>>>>>> invalid and will never succeed. > >>>>>>>>>>> > >>>>>>>>>>> However, attempting to categorize the proposed exceptions > >>>>>>>>>>> reveals even problems with this categorization: > >>>>>>>>>>> Retriable: > >>>>>>>>>>> * StreamsRebalancingException > >>>>>>>>>>> Recoverable: > >>>>>>>>>>> * StateStoreMigratedException > >>>>>>>>>>> * StreamsNotRunningException > >>>>>>>>>>> Fatal: > >>>>>>>>>>> * UnknownStateStoreException > >>>>>>>>>>> > >>>>>>>>>>> But StreamsNotStartedException is strange... It means that > >>>>>>>>>>> one code path got a handle on a specific KafkaStreams object > >>>>>>>>>>> instance and sent it a query before another code path > >>>>>>>>>>> invoked the start() method on the exact same object instance. > >>>>>>>>>>> It seems like the most likely scenario is that whoever wrote > >>>>>>>>>>> the program just forgot to call start() before querying, in > >>>>>>>>>>> which case, retrying isn't going to help, and a fatal exception > >>>>>>>>>>> is more appropriate. I.e., it sounds like a "first 15 minutes > >>>>>>>>>>> experience" problem, and making it fatal would be more > >>>>>>>>>>> helpful. Even in a production context, there's no reason not > >>>>>>>>>>> to sequence your application startup such that you don't > >>>>>>>>>>> accept queries until after Streams is started. Thus, I guess > >>>>>>>>>>> I'd categorize it under "fatal". > >>>>>>>>>>> > >>>>>>>>>>> Regardless of whether you make it fatal or retriable, you'd > >>>>>>>>>>> still have a whole category with only one exception in it, > >>>>>>>>>>> and the other two categories only have two exceptions. > >>>>>>>>>>> Plus, as you pointed out in the KIP, you can't get all > >>>>>>>>>>> exceptions in all cases anyway: > >>>>>>>>>>> * store() can only throw NotStarted, NotRunning, > >>>>>>>>>>> and Unknown > >>>>>>>>>>> * actual store queries can only throw Rebalancing, > >>>>>>>>>>> Migrated, and NotRunning > >>>>>>>>>>> > >>>>>>>>>>> Thus, in practice also, there are exactly three categories > >>>>>>>>>>> and also exactly three exception types. It doesn't seem > >>>>>>>>>>> like there's a great advantage to the categories here. To > >>>>>>>>>>> avoid the categorization problem and also to clarify what > >>>>>>>>>>> exceptions can actually be thrown in different circumstances, > >>>>>>>>>>> it seems like we should just: > >>>>>>>>>>> * get rid of the middle tier and make all the exceptions > >>>>>>>>>>> extend InvalidStateStoreException > >>>>>>>>>>> * drop StateStoreNotAvailableException in favor of > >>>>>>>>>>> StreamsNotRunningException > >>>>>>>>>>> * clearly document on all public methods which exceptions > >>>>>>>>>>> need to be handled > >>>>>>>>>>> > >>>>>>>>>>> How do you feel about this? > >>>>>>>>>>> Thanks, > >>>>>>>>>>> -John > >>>>>>>>>>> > >>>>>>>>>>> On Wed, Jan 15, 2020, at 15:13, Bill Bejeck wrote: > >>>>>>>>>>>> Thanks for KIP Vito. > >>>>>>>>>>>> > >>>>>>>>>>>> Overall the KIP LGTM, but I'd have to agree with others on > >> merging > >>>>>>>> the > >>>>>>>>>>>> `StreamsNotRunningException` and > >> `StateStoreNotAvailableException` > >>>>>>>>>>> classes. > >>>>>>>>>>>> > >>>>>>>>>>>> Since in both cases, the thread state is in `PENDING_SHUTDOWN > || > >>>>>>>>>>>> NOT_RUNNING || ERROR` I'm not even sure how we could > distinguish > >>>>>>>> when to > >>>>>>>>>>>> use the different > >>>>>>>>>>>> exceptions. Maybe a good middle ground would be to have a > >> detailed > >>>>>>>>>>>> exception message. > >>>>>>>>>>>> > >>>>>>>>>>>> The KIP freeze is close, so I think if we can agree on this, > we > >> can > >>>>>>>>>>> wrap up > >>>>>>>>>>>> the voting soon. > >>>>>>>>>>>> > >>>>>>>>>>>> Thanks, > >>>>>>>>>>>> Bill > >>>>>>>>>>>> > >>>>>>>>>>>> On Tue, Jan 14, 2020 at 2:12 PM Matthias J. Sax < > >>>>>>>> matth...@confluent.io> > >>>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> Vito, > >>>>>>>>>>>>> > >>>>>>>>>>>>> It's still unclear to me what the advantage is, to have both > >>>>>>>>>>>>> `StreamsNotRunningException` and > >>>>>>>> `StateStoreNotAvailableException`? > >>>>>>>>>>>>> > >>>>>>>>>>>>> For both cased, the state is `PENDING_SHUTDOWN / NOT_RUNNING > / > >>>>>>>> ERROR` > >>>>>>>>>>>>> and thus, for a user point of view, why does it matter if the > >>>>>>>> store is > >>>>>>>>>>>>> closed on not? I don't understand why/how this information > >> would > >>>>>>>> be > >>>>>>>>>>>>> useful? Do you have a concrete example in mind how a user > would > >>>>>>>> react > >>>>>>>>>>>>> differently to both exceptions? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> @Vinoth: about `StreamsRebalancingException` -- to me, it > seems > >>>>>>>> best > >>>>>>>>>>> to > >>>>>>>>>>>>> actually do this on a per-query basis, ie, have an overload > >>>>>>>>>>>>> `KafkaStreams#store(...)` that takes a boolean flag that > allow > >> to > >>>>>>>>>>>>> _disable_ the exception and opt-in to query a active store > >> during > >>>>>>>>>>>>> recovery. However, as KIP-535 actually introduces this change > >> in > >>>>>>>>>>>>> behavior, I think KIP-216 should not cover this, but KIP-535 > >>>>>>>> should be > >>>>>>>>>>>>> updated. I'll follow up on the other KIP thread to raise this > >>>>>>>> point. > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> -Matthias > >>>>>>>>>>>>> > >>>>>>>>>>>>> On 1/11/20 12:26 AM, Vito Jeng wrote: > >>>>>>>>>>>>>> Hi, Matthias & Vinoth, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Thanks for the feedback. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> What is still unclear to me is, what we gain by having both > >>>>>>>>>>>>>>> `StreamsNotRunningException` and > >>>>>>>>>>> `StateStoreNotAvailableException`. Both > >>>>>>>>>>>>>>> exception are thrown when KafkaStreams is in state > >>>>>>>>>>> PENDING_SHUTDOWN / > >>>>>>>>>>>>>>> NOT_RUNNING / ERROR. Hence, as a user what do I gain to > know > >>>>>>>> if the > >>>>>>>>>>>>>>> state store is closed on not -- I can't query it anyway? > >> Maybe > >>>>>>>> I > >>>>>>>>>>> miss > >>>>>>>>>>>>>>> something thought? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Yes, both `StreamsNotRunningException` and > >>>>>>>>>>>>>> `StateStoreNotAvailableException` are fatal exception. > >>>>>>>>>>>>>> But `StateStoreNotAvailableException` is fatal exception > about > >>>>>>>> state > >>>>>>>>>>>>> store > >>>>>>>>>>>>>> related. > >>>>>>>>>>>>>> I think it would be helpful that if user need to distinguish > >>>>>>>> these > >>>>>>>>>>> two > >>>>>>>>>>>>>> different case to handle it. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> I'm not very sure, does that make sense? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> Vito > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On Fri, Jan 10, 2020 at 3:35 AM Vinoth Chandar < > >>>>>>>> vin...@apache.org> > >>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> +1 on merging `StreamsNotRunningException` and > >>>>>>>>>>>>>>> `StateStoreNotAvailableException`, both exceptions are > fatal > >>>>>>>>>>> anyway. IMO > >>>>>>>>>>>>>>> its best to have these exceptions be about the state store > >>>>>>>> (and not > >>>>>>>>>>>>> streams > >>>>>>>>>>>>>>> state), to easier understanding. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Additionally, KIP-535 allows for querying of state stores > in > >>>>>>>>>>> rebalancing > >>>>>>>>>>>>>>> state. So do we need the StreamsRebalancingException? > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On 2020/01/09 03:38:11, "Matthias J. Sax" < > >>>>>>>> matth...@confluent.io> > >>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>> Sorry that I dropped the ball on this... > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thanks for updating the KIP. Overall LGTM now. Feel free > to > >>>>>>>> start > >>>>>>>>>>> a > >>>>>>>>>>>>> VOTE > >>>>>>>>>>>>>>>> thread. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> What is still unclear to me is, what we gain by having > both > >>>>>>>>>>>>>>>> `StreamsNotRunningException` and > >>>>>>>>>>> `StateStoreNotAvailableException`. > >>>>>>>>>>>>> Both > >>>>>>>>>>>>>>>> exception are thrown when KafkaStreams is in state > >>>>>>>>>>> PENDING_SHUTDOWN / > >>>>>>>>>>>>>>>> NOT_RUNNING / ERROR. Hence, as a user what do I gain to > know > >>>>>>>> if > >>>>>>>>>>> the > >>>>>>>>>>>>>>>> state store is closed on not -- I can't query it anyway? > >>>>>>>> Maybe I > >>>>>>>>>>> miss > >>>>>>>>>>>>>>>> something thought? > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> -Matthias > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> On 11/3/19 6:07 PM, Vito Jeng wrote: > >>>>>>>>>>>>>>>>> Sorry for the late reply, thanks for the review. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> About `StateStoreMigratedException`: > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Why is it only thrown if the state is REBALANCING? A > store > >>>>>>>>>>> might be > >>>>>>>>>>>>>>>>>> migrated during a rebalance, and Kafka Streams might > >> resume > >>>>>>>>>>> back to > >>>>>>>>>>>>>>>>>> RUNNING state and afterward somebody tries to use an old > >>>>>>>> store > >>>>>>>>>>>>> handle. > >>>>>>>>>>>>>>>>>> Also, if state is REBALANCING, should we throw > >>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException`? Hence, I think > >>>>>>>>>>>>>>>>>> `StateStoreMigratedException` does only make sense > during > >>>>>>>>>>> `RUNNING` > >>>>>>>>>>>>>>> state. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Thank you point this, already updated. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Why do we need to distinguish between > >>>>>>>>>>>>> `KafkaStreamsNotRunningException` > >>>>>>>>>>>>>>>>>> and `StateStoreNotAvailableException`? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` may be caused by > various > >>>>>>>>>>> reasons, I > >>>>>>>>>>>>>>> think > >>>>>>>>>>>>>>>>> it would be helpful that the > >>>>>>>>>>>>>>>>> user can distinguish whether it is caused by the state > >> store > >>>>>>>>>>> closed. > >>>>>>>>>>>>>>>>> (Maybe I am wrong...) > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Last, why do we distinguish between `KafkaStreams` > instance > >>>>>>>> and > >>>>>>>>>>>>>>>>>> `StreamsThread`? To me, it seems we should always refer > to > >>>>>>>> the > >>>>>>>>>>>>>>> instance, > >>>>>>>>>>>>>>>>>> because that is the level of granularity in which we > >>>>>>>>>>> enable/disable > >>>>>>>>>>>>>>> IQ atm. > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Totally agree. Do you mean the naming of state store > >>>>>>>> exceptions? > >>>>>>>>>>>>>>>>> I don't have special reason to distinguish these two. > >>>>>>>>>>>>>>>>> Your suggestion look more reasonable for the exception > >>>>>>>> naming. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Last, for `StateStoreMigratedException`, I would add > that a > >>>>>>>> user > >>>>>>>>>>> need > >>>>>>>>>>>>>>> to > >>>>>>>>>>>>>>>>>> rediscover the store and cannot blindly retry as the > store > >>>>>>>>>>> handle is > >>>>>>>>>>>>>>>>>> invalid and a new store handle must be retrieved. That > is > >> a > >>>>>>>>>>>>> difference > >>>>>>>>>>>>>>>>>> to `StreamThreadRebalancingException` that allows for > >>>>>>>> "blind" > >>>>>>>>>>> retries > >>>>>>>>>>>>>>>>>> that either resolve (if the store is still on the same > >>>>>>>> instance > >>>>>>>>>>> after > >>>>>>>>>>>>>>>>>> rebalancing finishes, or changes to > >>>>>>>>>>> `StateStoreMigratedException` if > >>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>> store was migrated away during rebalancing). > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> Nice, it's great! Thank you. > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> The KIP already updated, please take a look. :) > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> On Wed, Oct 23, 2019 at 1:48 PM Matthias J. Sax < > >>>>>>>>>>>>> matth...@confluent.io > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> Any update on this KIP? > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> On 10/7/19 3:35 PM, Matthias J. Sax wrote: > >>>>>>>>>>>>>>>>>>> Sorry for the late reply. The 2.4 deadline kept us > quite > >>>>>>>> busy. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> About `StateStoreMigratedException`: > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Why is it only thrown if the state is REBALANCING? A > >> store > >>>>>>>>>>> might be > >>>>>>>>>>>>>>>>>>> migrated during a rebalance, and Kafka Streams might > >> resume > >>>>>>>>>>> back to > >>>>>>>>>>>>>>>>>>> RUNNING state and afterward somebody tries to use an > old > >>>>>>>> store > >>>>>>>>>>>>>>> handle. > >>>>>>>>>>>>>>>>>>> Also, if state is REBALANCING, should we throw > >>>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException`? Hence, I think > >>>>>>>>>>>>>>>>>>> `StateStoreMigratedException` does only make sense > during > >>>>>>>>>>> `RUNNING` > >>>>>>>>>>>>>>>>>> state. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Why do we need to distinguish between > >>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` > >>>>>>>>>>>>>>>>>>> and `StateStoreNotAvailableException`? > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Last, why do we distinguish between `KafkaStreams` > >>>>>>>> instance and > >>>>>>>>>>>>>>>>>>> `StreamsThread`? To me, it seems we should always refer > >> to > >>>>>>>> the > >>>>>>>>>>>>>>> instance, > >>>>>>>>>>>>>>>>>>> because that is the level of granularity in which we > >>>>>>>>>>> enable/disable > >>>>>>>>>>>>>>> IQ > >>>>>>>>>>>>>>>>>> atm. > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> Last, for `StateStoreMigratedException`, I would add > >> that a > >>>>>>>>>>> user > >>>>>>>>>>>>>>> need to > >>>>>>>>>>>>>>>>>>> rediscover the store and cannot blindly retry as the > >> store > >>>>>>>>>>> handle is > >>>>>>>>>>>>>>>>>>> invalid and a new store handle must be retrieved. That > >> is a > >>>>>>>>>>>>>>> difference > >>>>>>>>>>>>>>>>>>> to `StreamThreadRebalancingException` that allows for > >>>>>>>> "blind" > >>>>>>>>>>>>> retries > >>>>>>>>>>>>>>>>>>> that either resolve (if the store is still on the same > >>>>>>>> instance > >>>>>>>>>>>>> after > >>>>>>>>>>>>>>>>>>> rebalancing finishes, or changes to > >>>>>>>>>>> `StateStoreMigratedException` if > >>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>>> store was migrated away during rebalancing). > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> -Matthias > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> On 8/9/19 10:20 AM, Vito Jeng wrote: > >>>>>>>>>>>>>>>>>>>> My bad. The short link `https://shorturl.at/CDNT9` > <https://shorturl.at/CDNT9> > >> <https://shorturl.at/CDNT9> > >>>>>> <https://shorturl.at/CDNT9> > >>>>>>>> <https://shorturl.at/CDNT9> > >>>>>>>>>>> <https://shorturl.at/CDNT9> > >>>>>>>>>>>>> <https://shorturl.at/CDNT9> > >>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> > >>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> > >>>>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> seems incorrect. > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> Please use the following instead: > >>>>>>>> https://shorturl.at/bkKQU > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> --- > >>>>>>>>>>>>>>>>>>>> Vito > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> On Fri, Aug 9, 2019 at 10:53 AM Vito Jeng < > >>>>>>>>>>> v...@is-land.com.tw> > >>>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> Thanks, Matthias! > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> About `StreamThreadNotStartedException`: > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> Thank you for explanation. I agree with your opinion. > >>>>>>>>>>>>>>>>>>>>> `CompositeReadOnlyXxxStore#get()` would never throw > >>>>>>>>>>>>>>>>>>>>> `StreamThreadNotStartedException`. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> For the case that corresponding thread crashes after > we > >>>>>>>>>>> handed out > >>>>>>>>>>>>>>> the > >>>>>>>>>>>>>>>>>>>>> store handle. We may throw > >>>>>>>> `KafkaStreamsNotRunningException` > >>>>>>>>>>> or > >>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`. > >>>>>>>>>>>>>>>>>>>>> In `StreamThreadStateStoreProvider`, we would throw > >>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` when stream thread > is > >>>>>>>> not > >>>>>>>>>>>>>>> running( > >>>>>>>>>>>>>>>>>>>>> https://shorturl.at/CDNT9) or throw > >>>>>>>>>>> `StateStoreMigratedException` > >>>>>>>>>>>>>>> when > >>>>>>>>>>>>>>>>>>>>> store is closed(https://shorturl.at/hrvAN). So I > think > >>>>>>>> we > >>>>>>>>>>> do not > >>>>>>>>>>>>>>> need > >>>>>>>>>>>>>>>>>> to > >>>>>>>>>>>>>>>>>>>>> add a new type for this case. Does that make sense? > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> About `KafkaStreamsNotRunningException` vs > >>>>>>>>>>>>>>>>>>>>> `StreamThreadNotRunningException`: > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> I understand your point. I rename > >>>>>>>>>>>>>>> `StreamThreadNotRunningException` to > >>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException`. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> About check unknown state store names: > >>>>>>>>>>>>>>>>>>>>> Thank you for the hint. I add a new type > >>>>>>>>>>>>>>> `UnknownStateStoreException` > >>>>>>>>>>>>>>>>>> for > >>>>>>>>>>>>>>>>>>>>> this case. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> Also, we should still have fatal exception > >>>>>>>>>>>>>>>>>>>>> `StateStoreNotAvailableException`? Not sure why you > >>>>>>>> remove > >>>>>>>>>>> it? > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> Thank you point this, already add it again. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> The KIP already updated, please take a look. > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>> --- > >>>>>>>>>>>>>>>>>>>>> Vito > >>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>> > >> > >> > > > >
