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, 05 Jul 2019 15:03:36 GMT
Hi, Mattias,

Just completed the modification of KIP, please take a look when you are
available.

---
Vito


On Wed, Jul 3, 2019 at 9:07 PM Vito Jeng <vito@is-land.com.tw> wrote:

> 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