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 Sat, 22 Jun 2019 07:31:22 GMT
Hi, Mattias,

Sorry for late reply.
Nice to hear from you, thank you for re-reading the discussion thread and
help to finish the KIP.
I spend some time rereading our past discussions and updating the KIP.
Hope the following reply can have more clarification.

> For example, StreamThreadNotStartedException, seems to only make sense
for `KafkaStreams#store()`?

StreamThreadNotStartedException not only thrown from `KafkaStreams#store()`
but also thrown from all public methods in the `ReadOnlyKeyValueStore` and
`ReadOnlySessionStore` and `ReadOnlyWindowStore` (class
CompositeReadOnlyKeyValueStore, CompositeReadOnlySessionStore,
CompositeReadOnlyWindowStore).

For example, in the `CompositeReadOnlyKeyValueStore#get(key)`, the first
step is calling `StateStoreProvider#stores()` to get all matched stores.
This may cause `StreamThreadNotStartedException`. Basically, all public
methods in the `ReadOnlyKeyValueStore` and `ReadOnlySessionStore` and
`ReadOnlyWindowStore` need to call `StateStoreProvider#stores()`. This
could be considered the same situation as `KafkaStreams#store()` calling
`QueryableStoreProvider#getStore()`

Does this make sense?


> For `StreamThreadNotRunningException` should we rename it to
`KafkaStreamsNotRunningException` ?

Hmmm...I am not sure. `KafkaStreamsNotRunningException` is a good name and
I like it. KafkaStreams running implies that all stream threads are
running.
But `StreamThreadNotRunningException` could be exactly to tell the user
what happened, IMHO.

I personally pick `StreamThreadNotRunningException`. But I am fine to pick
`KafkaStreamsNotRunningException` if you think this name is better. :)


> 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()` ?

Just like `StreamThreadNotStartedException`(my replied above), from my
current understanding,
the `StreamThreadNotRunningException` and `StateStoreNotAvailableException`
not only thrown from `KafkaStreams#store()` but also thrown from all public
methods in the `ReadOnlyKeyValueStore` and `ReadOnlySessionStore` and
`ReadOnlyWindowStore.

for `StateStoreNotAvailableException` -- It would be thrown if no such
store exists in the topology at all(EmptyStateStoreException) and would be
thrown when the store already closed(StateStoreClosedException).  for
`StreamThreadNotRunningException` - It would be thrown if the stream thread
is not running.

IMHO, it would be better that distinguish the two different exception so
that user can handle by different way.
What do you think?


During I wrote this letter, I have a question and would like to ask you for
clarification:
When the user passes a store name to `KafkaStreams#store()`, does there
have a way that distinguish the store name is "a wrong name" or "migrated"
during `QueryableStoreProvider#getStore()`? (
https://github.com/apache/kafka/blob/11641c7f53b2ee156490b2aadf79552d266ec956/streams/src/main/java/org/apache/kafka/streams/state/internals/QueryableStoreProvider.java#L61-L63
)  From my current understanding, I cannot distinguish these two.

I will reply with the rest of the questions in another letter.
Thank you again.

---
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