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