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