kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Bejeck <bbej...@gmail.com>
Subject Re: [DISCUSS]KIP-216: IQ should throw different exceptions for different errors
Date Wed, 15 Jan 2020 21:13:32 GMT
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 <matthias@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 <vinoth@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" <matthias@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 <
> matthias@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> seems incorrect.
> >>>>>>>
> >>>>>>> Please use the following instead: https://shorturl.at/bkKQU
> >>>>>>>
> >>>>>>>
> >>>>>>> ---
> >>>>>>> Vito
> >>>>>>>
> >>>>>>>
> >>>>>>> On Fri, Aug 9, 2019 at 10:53 AM Vito Jeng <vito@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
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>
> >
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message