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 Sun, 11 Nov 2018 09:31:02 GMT
Hi, Matthias,

Sorry for the late reply.

> I am wondering what the semantic impact/change is, if we introduce
> `RetryableStateStoreException` and `FatalStateStoreException` that both
> inherit from it. While I like the introduction of both from a high level
> point of view, I just want to make sure it's semantically sound and
> backward compatible. Atm, I think it's fine, but I want to point it out
> such that everybody can think about this, too, so we can verify that
> it's a natural evolving API change.

Thank you for pointing this out. This's really important for public API.

Just when I was replying to you, I found that KIP needs some modify.
I will fix it ASAP, and then let's continue the discussion.

---
Vito


On Wed, Nov 7, 2018 at 7:06 AM Matthias J. Sax <matthias@confluent.io>
wrote:

> Hey Vito,
>
> I saw that you updated your PR, but did not reply to my last comments.
> Any thoughts?
>
>
> -Matthias
>
> On 10/19/18 10:34 AM, Matthias J. Sax wrote:
> > Glad to have you back Vito :)
> >
> > Some follow up thoughts:
> >
> >  - the current `InvalidStateStoreException` is documents as being
> > sometimes retry-able. From the JavaDocs:
> >
> >> These exceptions may be transient [...] Hence, it is valid to backoff
> and retry when handling this exception.
> >
> > I am wondering what the semantic impact/change is, if we introduce
> > `RetryableStateStoreException` and `FatalStateStoreException` that both
> > inherit from it. While I like the introduction of both from a high level
> > point of view, I just want to make sure it's semantically sound and
> > backward compatible. Atm, I think it's fine, but I want to point it out
> > such that everybody can think about this, too, so we can verify that
> > it's a natural evolving API change.
> >
> >  - StateStoreClosedException:
> >
> >> will be wrapped to StateStoreMigratedException or
> StateStoreNotAvailableException later.
> >
> > Can you clarify the cases (ie, when will it be wrapped with the one or
> > the other)?
> >
> >  - StateStoreIsEmptyException:
> >
> > I don't understand the semantic of this exception. Maybe it's a naming
> > issue?
> >
> >> will be wrapped to StateStoreMigratedException or
> StateStoreNotAvailableException later.
> >
> > Also, can you clarify the cases (ie, when will it be wrapped with the
> > one or the other)?
> >
> >
> > I am also wondering, if we should introduce a fatal exception
> > `UnkownStateStoreException` to tell users that they passed in an unknown
> > store name?
> >
> >
> >
> > -Matthias
> >
> >
> >
> > On 10/17/18 8:14 PM, vito jeng wrote:
> >> Just open a PR for further discussion:
> >> https://github.com/apache/kafka/pull/5814
> >>
> >> Any suggestion is welcome.
> >> Thanks!
> >>
> >> ---
> >> Vito
> >>
> >>
> >> On Thu, Oct 11, 2018 at 12:14 AM vito jeng <vito@is-land.com.tw> wrote:
> >>
> >>> Hi John,
> >>>
> >>> Thanks for reviewing the KIP.
> >>>
> >>>> I didn't follow the addition of a new method to the QueryableStoreType
> >>>> interface. Can you elaborate why this is necessary to support the new
> >>>> exception types?
> >>>
> >>> To support the new exception types, I would check stream state in the
> >>> following classes:
> >>>   - CompositeReadOnlyKeyValueStore class
> >>>   - CompositeReadOnlySessionStore class
> >>>   - CompositeReadOnlyWindowStore class
> >>>   - DelegatingPeekingKeyValueIterator class
> >>>
> >>> It is also necessary to keep backward compatibility. So I plan passing
> >>> stream
> >>> instance to QueryableStoreType instance during KafkaStreams#store()
> >>> invoked.
> >>> It looks a most simple way, I think.
> >>>
> >>> It is why I add a new method to the QueryableStoreType interface. I can
> >>> understand
> >>> that we should try to avoid adding the public api method. However, at
> the
> >>> moment
> >>> I have no better ideas.
> >>>
> >>> Any thoughts?
> >>>
> >>>
> >>>> Also, looking over your KIP again, it seems valuable to introduce
> >>>> "retriable store exception" and "fatal store exception" marker
> interfaces
> >>>> that the various exceptions can mix in. It would be nice from a
> usability
> >>>> perspective to be able to just log and retry on any "retriable"
> exception
> >>>> and log and shutdown on any fatal exception.
> >>>
> >>> I agree that this is valuable to the user.
> >>> I'll update the KIP.
> >>>
> >>>
> >>> Thanks
> >>>
> >>>
> >>> ---
> >>> Vito
> >>>
> >>>
> >>> On Tue, Oct 9, 2018 at 2:30 AM John Roesler <john@confluent.io> wrote:
> >>>
> >>>> Hi Vito,
> >>>>
> >>>> I'm glad to hear you're well again!
> >>>>
> >>>> I didn't follow the addition of a new method to the QueryableStoreType
> >>>> interface. Can you elaborate why this is necessary to support the new
> >>>> exception types?
> >>>>
> >>>> Also, looking over your KIP again, it seems valuable to introduce
> >>>> "retriable store exception" and "fatal store exception" marker
> interfaces
> >>>> that the various exceptions can mix in. It would be nice from a
> usability
> >>>> perspective to be able to just log and retry on any "retriable"
> exception
> >>>> and log and shutdown on any fatal exception.
> >>>>
> >>>> Thanks,
> >>>> -John
> >>>>
> >>>> On Fri, Oct 5, 2018 at 11:47 AM Guozhang Wang <wangguoz@gmail.com>
> wrote:
> >>>>
> >>>>> Thanks for the explanation, that makes sense.
> >>>>>
> >>>>>
> >>>>> Guozhang
> >>>>>
> >>>>>
> >>>>> On Mon, Jun 25, 2018 at 2:28 PM, Matthias J. Sax <
> matthias@confluent.io
> >>>>>
> >>>>> wrote:
> >>>>>
> >>>>>> The scenario I had I mind was, that KS is started in one thread
> while
> >>>> a
> >>>>>> second thread has a reference to the object to issue queries.
> >>>>>>
> >>>>>> If a query is issue before the "main thread" started KS, and the
> >>>> "query
> >>>>>> thread" knows that it will eventually get started, it can retry. On
> >>>> the
> >>>>>> other hand, if KS is in state PENDING_SHUTDOWN or DEAD, it is
> >>>> impossible
> >>>>>> to issue any query against it now or in the future and thus the
> error
> >>>> is
> >>>>>> not retryable.
> >>>>>>
> >>>>>>
> >>>>>> -Matthias
> >>>>>>
> >>>>>> On 6/25/18 10:15 AM, Guozhang Wang wrote:
> >>>>>>> I'm wondering if StreamThreadNotStarted could be merged into
> >>>>>>> StreamThreadNotRunning, because I think users' handling logic for
> >>>> the
> >>>>>> third
> >>>>>>> case would be likely the same as the second. Do you have some
> >>>> scenarios
> >>>>>>> where users may want to handle them differently?
> >>>>>>>
> >>>>>>> Guozhang
> >>>>>>>
> >>>>>>> On Sun, Jun 24, 2018 at 5:25 PM, Matthias J. Sax <
> >>>>> matthias@confluent.io>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Sorry to hear! Get well soon!
> >>>>>>>>
> >>>>>>>> It's not a big deal if the KIP stalls a little bit. Feel free to
> >>>> pick
> >>>>> it
> >>>>>>>> up again when you find time.
> >>>>>>>>
> >>>>>>>>>>> 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.
> >>>>>>>>
> >>>>>>>> I see. If this is the intention, than I would suggest to have two
> >>>> (or
> >>>>>>>> maybe three) different exceptions:
> >>>>>>>>
> >>>>>>>>  - StreamThreadRebalancingException (retryable)
> >>>>>>>>  - StreamThreadNotRunning (not retryable -- thrown if in state
> >>>>>>>> PENDING_SHUTDOWN or DEAD
> >>>>>>>>  - maybe StreamThreadNotStarted (for state CREATED)
> >>>>>>>>
> >>>>>>>> The last one is tricky and could also be merged into one of the
> >>>> first
> >>>>>>>> two, depending if you want to argue that it's retryable or not.
> >>>> (Just
> >>>>>>>> food for though -- not sure what others think.)
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> -Matthias
> >>>>>>>>
> >>>>>>>> On 6/22/18 8:06 AM, vito jeng wrote:
> >>>>>>>>> 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
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> -- Guozhang
> >>>>>
> >>>>
> >>>
> >>
> >
>
>

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