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 <v...@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 <matth...@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 <v...@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 <j...@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 <wangg...@gmail.com>
>>> wrote:
>>>>>>>
>>>>>>>> Thanks for the explanation, that makes sense.
>>>>>>>>
>>>>>>>>
>>>>>>>> Guozhang
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Jun 25, 2018 at 2:28 PM, Matthias J. Sax <
>>> matth...@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 <
>>>>>>>> matth...@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 <
>>>>>>>>> matth...@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 <
>>> bbej...@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 <
>>>>>>>> wangg...@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 <
>>>>>>> j...@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 <
>>>>>>>>>>>>>>> matth...@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 <
>>>>>>>>>>>>>>>>> matth...@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 <
>>>>>>>>>>>>>>>>> matth...@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 <
>>>>>>>>>>>>>>>>>>>> matth...@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 <
>>>>>>>>>>>>>>>>>>>>>> matth...@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
>>> <
>>>>>>>>>>>>>>>>>>>>>>>> matth...@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 <
>>>>>>>>>>>>>>>>>> v...@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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
> 

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to