kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [DISCUSS]KIP-216: IQ should throw different exceptions for different errors
Date Tue, 06 Nov 2018 23:06:33 GMT
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
View raw message