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