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 Wed, 03 Jul 2019 13:07:31 GMT
Hi, Matthias,

This is second part.

> For the internal exceptions:
>
> `StateStoreClosedException` -- why can it be wrapped as
> `StreamThreadNotStartedException` ? It seems that the later would only
> be thrown by `KafkaStreams#store()` and thus would be throw directly.

Both `StateStoreClosedException` and `EmptyStateStoreException` not can be
wrapped as `StreamThreadNotStartedException`.
This is a mistaken written in the previous KIP. Thank you point this.

> A closed-exception should only happen after a store was successfully
> retrieved but cannot be queried any longer? Hence, converting/wrapping
> it into a `StateStoreMigratedException` make sense. I am also not sure,
> when a closed-exception would be wrapped by a
> `StateStoreNotAvailableException` (implying my understanding as describe
> above)?
>
> Same questions about `EmptyStateStoreException`.
>
> Thinking about both internal exceptions twice, I am wondering if it
> makes sense to have both internal exceptions at all? I have the
> impression that it make only sense to wrap them with a
> `StateStoreMigragedException`, but if they are wrapped into the same
> exception all the time, we can just remove both and throw
> `StateStoreMigratedException` directly?

After deeper thinking, I think you are right. It seems we can throw
`StateStoreMigratedException` directly.
So that we can remove `StateStoreClosedException`,
`EmptyStateStoreException` and `StateStoreNotAvailableException`.
Will update the KIP.

BTW, if we remove above three exceptions, the
`StreamThreadNotRunningException` will be the only one sub class extends
from FatalStateStoreException.
Should we remove `StreamThreadNotRunningException` and throw
`FatalStateStoreException` directly ?

> Last point: Why do we need to add?
> QueryableStoreType#setStreams(KafkaStreams streams);
> John asked this question already and you replied to it. But I am not
> sure what your answer means. Can you explain it in more detail?

The main purpose is to pass the KafkaStreams reference into
CompositeReadOnlyKeyValueStore / CompositeReadOnlySessionStore/
CompositeReadOnlyWindowStore instance.
We need check KafkaStreams state to warp InvalidStateStoreException in to
other exception(e.g., StateStoreMigratedException) when the user accesses
these read-only stores.

The original thought is to add `setStreams` method in to
QueryableStoreType. But now I think I find a better way during recent days.
This way does not need to change any public interface. So we can skip this
question. :)


I will update the KIP based on our discussion.
Thank you for help to finish the KIP!

---
Vito


On Thu, Jun 6, 2019 at 8:23 AM Matthias J. Sax <matthias@confluent.io>
wrote:

> Hi Vito,
>
> sorry for dropping this discussion on the floor a while back. I was just
> re-reading the KIP and discussion thread, and I think it is shaping out
> nicely!
>
> I like the overall hierarchy of the exception classes.
>
> Some things are still not 100% clear:
>
>
> You listed all methods that may throw an `InvalidStateStoreException`
> atm. For the new exceptions, can any exception be thrown by any method?
> It might help to understand this relationship better.
>
> For example, StreamThreadNotStartedException, seems to only make sense
> for `KafkaStreams#store()`?
>
>
> For `StreamThreadNotRunningException` should we rename it to
> `KafkaStreamsNotRunningException` ?
>
>
> The description of `StreamThreadNotRunningException` and
> `StateStoreNotAvailableException` seems to be the same? From my
> understandng, the description makes sense for
> `StreamThreadNotRunningException` -- for
> `StateStoreNotAvailableException` I was expecting/inferring from the
> name, that it would be thrown if no such store exists in the topology at
> all (ie, user passed in a invalid/wrong store name). For this case, this
> exception should be thrown only from `KafkaStreams#store()` ?
>
>
> For the internal exceptions:
>
> `StateStoreClosedException` -- why can it be wrapped as
> `StreamThreadNotStartedException` ? It seems that the later would only
> be thrown by `KafkaStreams#store()` and thus would be throw directly. A
> closed-exception should only happen after a store was successfully
> retrieved but cannot be queried any longer? Hence, converting/wrapping
> it into a `StateStoreMigratedException` make sense. I am also not sure,
> when a closed-exception would be wrapped by a
> `StateStoreNotAvailableException` (implying my understanding as describe
> above)?
>
> Same questions about `EmptyStateStoreException`.
>
> Thinking about both internal exceptions twice, I am wondering if it
> makes sense to have both internal exceptions at all? I have the
> impression that it make only sense to wrap them with a
> `StateStoreMigragedException`, but if they are wrapped into the same
> exception all the time, we can just remove both and throw
> `StateStoreMigratedException` directly?
>
>
> Last point: Why do we need to add?
>
> > QueryableStoreType#setStreams(KafkaStreams streams);
>
> John asked this question already and you replied to it. But I am not
> sure what your answer means. Can you explain it in more detail?
>
>
>
> Thanks for your patience on this KIP!
>
>
>
> -Matthias
>
>
>
>
>
>
> On 11/11/18 4:55 AM, Vito Jeng wrote:
> > Hi, Matthias,
> >
> > KIP already updated.
> >
> >> - 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)?
> >
> > For example, in the implementation(CompositeReadOnlyKeyValueStore#get),
> we
> > get all stores first, and then call ReadOnlyKeyValueStore#get to get
> value
> > in every store iteration.
> >
> > When calling ReadOnlyKeyValueStore#get, the StateStoreClosedException
> will
> > be thrown if the state store is not open.
> > We need catch StateStoreClosedException and wrap it in different
> exception
> > type:
> >   * If the stream's state is CREATED, we wrap StateStoreClosedException
> > with StreamThreadNotStartedException. User can retry until to RUNNING.
> >   * If the stream's state is RUNNING / REBALANCING, the state store
> should
> > be migrated, we wrap StateStoreClosedException with
> > StateStoreMigratedException. User can rediscover the state store.
> >   * If the stream's state is PENDING_SHUTDOWN / NOT_RUNNING / ERROR, the
> > stream thread is not available, we wrap StateStoreClosedException with
> > StateStoreNotAvailableException. User cannot retry when this exception is
> > thrown.
> >
> >
> >> - StateStoreIsEmptyException:
> >>  I don't understand the semantic of this exception. Maybe it's a naming
> > issue?
> >
> > I think yes. :)
> > Does `EmptyStateStoreException` is better ? (already updated in the KIP)
> >
> >
> >> - StateStoreIsEmptyException:
> >> 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)?
> >
> > For example, in the implementation (CompositeReadOnlyKeyValueStore#get),
> we
> > call StateStoreProvider#stores (WrappingStoreProvider#stores) to get all
> > stores. EmptyStateStoreException will be thrown when cannot find any
> store
> > and then we need catch it and wrap it in different exception type:
> >   * If the stream's state is CREATED, we wrap EmptyStateStoreException
> with
> > StreamThreadNotStartedException. User can retry until to RUNNING.
> >   * If the stream's state is RUNNING / REBALANCING, the state store
> should
> > be migrated, we wrap EmptyStateStoreException with
> > StateStoreMigratedException. User can rediscover the state store.
> >   * If the stream's state is PENDING_SHUTDOWN / NOT_RUNNING / ERROR, the
> > stream thread is not available, we wrap EmptyStateStoreException with
> > StateStoreNotAvailableException. User cannot retry when this exception is
> > thrown.
> >
> > I hope the above reply can clarify.
> >
> > The last one that was not replied was:
> >
> >> I am also wondering, if we should introduce a fatal exception
> >> `UnkownStateStoreException` to tell users that they passed in an unknown
> >> store name?
> >
> > Until now, unknown state store is not thinking about in the KIP.
> > I believe it would be very useful for users.
> >
> > Looking at the related code(WrappingStoreProvider#stores),
> > I found that I can't distinguish between the state store was migrated or
> an
> > unknown state store.
> >
> > Any thoughts?
> >
> > ---
> > Vito
> >
> >
> >
> > On Sun, Nov 11, 2018 at 5:31 PM Vito Jeng <vito@is-land.com.tw> wrote:
> >
> >> 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