kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From vito jeng <v...@is-land.com.tw>
Subject Re: [DISCUSS]KIP-216: IQ should throw different exceptions for different errors
Date Fri, 22 Jun 2018 15:06:25 GMT
Matthias,

Thank you for your assistance.

> what is the status of this KIP?

Unfortunately, there is no further progress.
About seven weeks ago, I was injured in sports. I had a broken wrist on
my left wrist.
Many jobs are affected, including this KIP and implementation.


> I just re-read it, and have a couple of follow up comments. Why do we
> discuss the internal exceptions you want to add? Also, do we really need
> them? Can't we just throw the correct exception directly instead of
> wrapping it later?

I think you may be right. As I say in the previous:
"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."

During the implementation, I also feel we maybe not need wrapper it.
We can just throw the correctly directly.


> Is `StreamThreadNotRunningException` really an retryable error?

When KafkaStream state is REBALANCING, I think it is a retryable error.

StreamThreadStateStoreProvider#stores() will throw
StreamThreadNotRunningException when StreamThread state is not RUNNING. The
user can retry until KafkaStream state is RUNNING.


> When would we throw an `StateStoreEmptyException`? The semantics is
unclear to me atm.

> When the state is RUNNING, is `StateStoreClosedException` a retryable
error?

These two comments will be answered in another mail.



---
Vito

On Mon, Jun 11, 2018 at 8:12 AM, Matthias J. Sax <matthias@confluent.io>
wrote:

> Vito,
>
> what is the status of this KIP?
>
> I just re-read it, and have a couple of follow up comments. Why do we
> discuss the internal exceptions you want to add? Also, do we really need
> them? Can't we just throw the correct exception directly instead of
> wrapping it later?
>
> When would we throw an `StateStoreEmptyException`? The semantics is
> unclear to me atm.
>
> Is `StreamThreadNotRunningException` really an retryable error?
>
> When the state is RUNNING, is `StateStoreClosedException` a retryable
> error?
>
> One more nits: ReadOnlyWindowStore got a new method #fetch(K key, long
> time); that should be added
>
>
> Overall I like the KIP but some details are still unclear. Maybe it
> might help if you open an PR in parallel?
>
>
> -Matthias
>
> On 4/24/18 8:18 AM, vito jeng wrote:
> > Hi, Guozhang,
> >
> > Thanks for the comment!
> >
> >
> >
> > Hi, Bill,
> >
> > I'll try to make some update to make the KIP better.
> >
> > Thanks for the comment!
> >
> >
> > ---
> > Vito
> >
> > On Sat, Apr 21, 2018 at 5:40 AM, Bill Bejeck <bbejeck@gmail.com> wrote:
> >
> >> Hi Vito,
> >>
> >> Thanks for the KIP, overall it's a +1 from me.
> >>
> >> At this point, the only thing I would change is possibly removing the
> >> listing of all methods called by the user and the listing of all store
> >> types and focus on what states result in which exceptions thrown to the
> >> user.
> >>
> >> Thanks,
> >> Bill
> >>
> >> On Fri, Apr 20, 2018 at 2:10 PM, Guozhang Wang <wangguoz@gmail.com>
> wrote:
> >>
> >>> Thanks for the KIP Vito!
> >>>
> >>> I made a pass over the wiki and it looks great to me. I'm +1 on the
> KIP.
> >>>
> >>> About the base class InvalidStateStoreException itself, I'd actually
> >>> suggest we do not deprecate it but still expose it as part of the
> public
> >>> API, for people who do not want to handle these cases differently (if
> we
> >>> deprecate it then we are enforcing them to capture all three exceptions
> >>> one-by-one).
> >>>
> >>>
> >>> Guozhang
> >>>
> >>>
> >>> On Fri, Apr 20, 2018 at 9:14 AM, John Roesler <john@confluent.io>
> wrote:
> >>>
> >>>> 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 `StateStoreStreamThreadNotRunni
> >>>> ngException`
> >>>>>>>>> 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
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>>
> >>>
> >>> --
> >>> -- Guozhang
> >>>
> >>
> >
>
>

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