> 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> 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 >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
signature.asc
Description: OpenPGP digital signature
