kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Roesler <j...@confluent.io>
Subject Re: [DISCUSS]KIP-216: IQ should throw different exceptions for different errors
Date Fri, 20 Apr 2018 16:14:56 GMT
Hi Vito,

Thanks for the KIP!

I think it's much nicer to give callers different exceptions to tell them
whether the state store got migrated, whether it's still initializing, or
whether there's some unrecoverable error.

In the KIP, it's typically not necessary to discuss non-user-facing details
such as what exceptions we will throw internally. The KIP is primarily to
discuss public interface changes.

You might consider simply removing all the internal details from the KIP,
which will have the dual advantage that it makes the KIP smaller and easier
to agree on, as well as giving you more freedom in the internal details
when it comes to implementation.

I like your decision to have your refined exceptions extend
InvalidStateStoreException to ensure backward compatibility. Since we want
to encourage callers to catch the more specific exceptions, and we don't
expect to ever throw a raw InvalidStateStoreException anymore, you might
consider adding the @Deprecated annotation to InvalidStateStoreException.
This will gently encourage callers to migrate to the new exception and open
the possibility of removing InvalidStateStoreException entirely in a future
release.

Thanks,
-John

On Thu, Apr 19, 2018 at 8:58 AM, Matthias J. Sax <matthias@confluent.io>
wrote:

> Thanks for clarification! That makes sense to me.
>
> Can you update the KIP to make those suggestions explicit?
>
>
> -Matthias
>
> On 4/18/18 2:19 PM, vito jeng wrote:
> > Matthias,
> >
> > Thanks for the feedback!
> >
> >> It's up to you to keep the details part in the KIP or not.
> >
> > Got it!
> >
> >> The (incomplete) question was, if we need `StateStoreFailException` or
> >> if existing `InvalidStateStoreException` could be used? Do you suggest
> >> that `InvalidStateStoreException` is not thrown at all anymore, but only
> >> the new sub-classes (just to get a better understanding).
> >
> > Yes. I suggest that `InvalidStateStoreException` is not thrown at all
> > anymore,
> > but only new sub-classes.
> >
> >> Not sure what this sentence means:
> >>> The internal exception will be wrapped as category exception finally.
> >> Can you elaborate?
> >
> > For example, `StreamThreadStateStoreProvider#stores()` will throw
> > `StreamThreadNotRunningException`(internal exception).
> > And then the internal exception will be wrapped as
> > `StateStoreRetryableException` or `StateStoreFailException` during the
> > `KafkaStreams.store()` and throw to the user.
> >
> >
> >> Can you explain the purpose of the "internal exceptions". It's unclear
> > to me atm why they are introduced.
> >
> > Hmmm...the purpose of the "internal exceptions" is to distinguish between
> > the different kinds of InvalidStateStoreException.
> > The original idea is that we can distinguish different state store
> > exception for
> > different handling.
> > But to be honest, I am not quite sure this is necessary. Maybe have
> > some change during implementation.
> >
> > Does it make sense?
> >
> >
> >
> >
> > ---
> > Vito
> >
> > On Mon, Apr 16, 2018 at 5:59 PM, Matthias J. Sax <matthias@confluent.io>
> > wrote:
> >
> >> Thanks for the update Vito!
> >>
> >> It's up to you to keep the details part in the KIP or not.
> >>
> >>
> >> The (incomplete) question was, if we need `StateStoreFailException` or
> >> if existing `InvalidStateStoreException` could be used? Do you suggest
> >> that `InvalidStateStoreException` is not thrown at all anymore, but only
> >> the new sub-classes (just to get a better understanding).
> >>
> >>
> >> Not sure what this sentence means:
> >>
> >>> The internal exception will be wrapped as category exception finally.
> >>
> >> Can you elaborate?
> >>
> >>
> >> Can you explain the purpose of the "internal exceptions". It's unclear
> >> to me atm why they are introduced.
> >>
> >>
> >> -Matthias
> >>
> >> On 4/10/18 12:33 AM, vito jeng wrote:
> >>> Matthias,
> >>>
> >>> Thanks for the review.
> >>> I reply separately in the following sections.
> >>>
> >>>
> >>> ---
> >>> Vito
> >>>
> >>> On Sun, Apr 8, 2018 at 1:30 PM, Matthias J. Sax <matthias@confluent.io
> >
> >>> wrote:
> >>>
> >>>> Thanks for updating the KIP and sorry for the long pause...
> >>>>
> >>>> Seems you did a very thorough investigation of the code. It's useful
> to
> >>>> understand what user facing interfaces are affected.
> >>>
> >>> (Some parts might be even too detailed for a KIP.)
> >>>>
> >>>
> >>> I also think too detailed. Especially the section `Changes in call
> >> trace`.
> >>> Do you think it should be removed?
> >>>
> >>>
> >>>>
> >>>> To summarize my current understanding of your KIP, the main change is
> to
> >>>> introduce new exceptions that extend `InvalidStateStoreException`.
> >>>>
> >>>
> >>> yep. Keep compatibility in this KIP is important things.
> >>> I think the best way is that all new exceptions extend from
> >>> `InvalidStateStoreException`.
> >>>
> >>>
> >>>>
> >>>> Some questions:
> >>>>
> >>>>  - Why do we need ```? Could `InvalidStateStoreException` be used for
> >>>> this purpose?
> >>>>
> >>>
> >>> Does this question miss some word?
> >>>
> >>>
> >>>>
> >>>>  - What the superclass of `StateStoreStreamThreadNotRunningException`
> >>>> is? Should it be `InvalidStateStoreException` or
> >> `StateStoreFailException`
> >>>> ?
> >>>>
> >>>>  - Is `StateStoreClosed` a fatal or retryable exception ?
> >>>>
> >>>>
> >>> I apologize for not well written parts. I tried to modify some code in
> >> the
> >>> recent period and modify the KIP.
> >>> The modification is now complete. Please look again.
> >>>
> >>>
> >>>>
> >>>>
> >>>> -Matthias
> >>>>
> >>>>
> >>>> On 2/21/18 5:15 PM, vito jeng wrote:
> >>>>> Matthias,
> >>>>>
> >>>>> Sorry for not response these days.
> >>>>> I just finished it. Please have a look. :)
> >>>>>
> >>>>>
> >>>>>
> >>>>> ---
> >>>>> Vito
> >>>>>
> >>>>> On Wed, Feb 14, 2018 at 5:45 AM, Matthias J. Sax <
> >> matthias@confluent.io>
> >>>>> wrote:
> >>>>>
> >>>>>> Is there any update on this KIP?
> >>>>>>
> >>>>>> -Matthias
> >>>>>>
> >>>>>> On 1/3/18 12:59 AM, vito jeng wrote:
> >>>>>>> Matthias,
> >>>>>>>
> >>>>>>> Thank you for your response.
> >>>>>>>
> >>>>>>> I think you are right. We need to look at the state both
of
> >>>>>>> KafkaStreams and StreamThread.
> >>>>>>>
> >>>>>>> After further understanding of KafkaStreams thread and state
store,
> >>>>>>> I am currently rewriting the KIP.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> ---
> >>>>>>> Vito
> >>>>>>>
> >>>>>>> On Fri, Dec 29, 2017 at 4:32 AM, Matthias J. Sax <
> >>>> matthias@confluent.io>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Vito,
> >>>>>>>>
> >>>>>>>> Sorry for this late reply.
> >>>>>>>>
> >>>>>>>> There can be two cases:
> >>>>>>>>
> >>>>>>>>  - either a store got migrated way and thus, is not
hosted an the
> >>>>>>>> application instance anymore,
> >>>>>>>>  - or, a store is hosted but the instance is in state
rebalance
> >>>>>>>>
> >>>>>>>> For the first case, users need to rediscover the store.
For the
> >> second
> >>>>>>>> case, they need to wait until rebalance is finished.
> >>>>>>>>
> >>>>>>>> If KafkaStreams is in state ERROR, PENDING_SHUTDOWN,
or
> NOT_RUNNING,
> >>>>>>>> uses cannot query at all and thus they cannot rediscover
or retry.
> >>>>>>>>
> >>>>>>>> Does this make sense?
> >>>>>>>>
> >>>>>>>> -Matthias
> >>>>>>>>
> >>>>>>>> On 12/20/17 12:54 AM, vito jeng wrote:
> >>>>>>>>> Matthias,
> >>>>>>>>>
> >>>>>>>>> I try to clarify some concept.
> >>>>>>>>>
> >>>>>>>>> When streams state is REBALANCING, it means the
user can just
> plain
> >>>>>>>> retry.
> >>>>>>>>>
> >>>>>>>>> When streams state is ERROR or PENDING_SHUTDOWN
or NOT_RUNNING,
> it
> >>>>>> means
> >>>>>>>>> state store migrated to another instance, the user
needs to
> >>>> rediscover
> >>>>>>>> the
> >>>>>>>>> store.
> >>>>>>>>>
> >>>>>>>>> Is my understanding correct?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> ---
> >>>>>>>>> Vito
> >>>>>>>>>
> >>>>>>>>> On Sun, Nov 5, 2017 at 12:30 AM, Matthias J. Sax
<
> >>>>>> matthias@confluent.io>
> >>>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> Thanks for the KIP Vito!
> >>>>>>>>>>
> >>>>>>>>>> I agree with what Guozhang said. The original
idea of the Jira
> >> was,
> >>>> to
> >>>>>>>>>> give different exceptions for different "recovery"
strategies to
> >> the
> >>>>>>>> user.
> >>>>>>>>>>
> >>>>>>>>>> For example, if a store is currently recreated,
a user just need
> >> to
> >>>>>> wait
> >>>>>>>>>> and can query the store later. On the other
hand, if a store go
> >>>>>> migrated
> >>>>>>>>>> to another instance, a user needs to rediscover
the store
> instead
> >>>> of a
> >>>>>>>>>> "plain retry".
> >>>>>>>>>>
> >>>>>>>>>> Fatal errors might be a third category.
> >>>>>>>>>>
> >>>>>>>>>> Not sure if there is something else?
> >>>>>>>>>>
> >>>>>>>>>> Anyway, the KIP should contain a section that
talks about this
> >> ideas
> >>>>>> and
> >>>>>>>>>> reasoning.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> -Matthias
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> On 11/3/17 11:26 PM, Guozhang Wang wrote:
> >>>>>>>>>>> Thanks for writing up the KIP.
> >>>>>>>>>>>
> >>>>>>>>>>> Vito, Matthias: one thing that I wanted
to figure out first is
> >> what
> >>>>>>>>>>> categories of errors we want to notify the
users, if we only
> >> wants
> >>>> to
> >>>>>>>>>>> distinguish fatal v.s. retriable then probably
we should rename
> >> the
> >>>>>>>>>>> proposed StateStoreMigratedException /
> StateStoreClosedException
> >>>>>>>> classes.
> >>>>>>>>>>> And then from there we should list what
are the possible
> internal
> >>>>>>>>>>> exceptions ever thrown in those APIs in
the call trace, and
> which
> >>>>>>>>>>> exceptions should be wrapped to what others,
and which ones
> >> should
> >>>> be
> >>>>>>>>>>> handled without re-throwing, and which ones
should not be
> wrapped
> >>>> at
> >>>>>>>> all
> >>>>>>>>>>> but directly thrown to user's face.
> >>>>>>>>>>>
> >>>>>>>>>>> Guozhang
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Nov 1, 2017 at 11:09 PM, vito jeng
<
> vito@is-land.com.tw>
> >>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Hi,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'd like to start discuss KIP-216:
> >>>>>>>>>>>>
> >>>>>>>>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>>>>>>>>>>> 216%3A+IQ+should+throw+different+exceptions+for+
> >> different+errors
> >>>>>>>>>>>>
> >>>>>>>>>>>> Please have a look.
> >>>>>>>>>>>> Thanks!
> >>>>>>>>>>>>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>> Vito
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
> >>
> >
>
>

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