Any update on this KIP Vito?
On 7/11/19 4:26 PM, Matthias J. Sax wrote: > Thanks Vito! I think the KIP shapes out nicely! > > > To answer the open question you raised (I also adjust my answers based > on the latest KIP update) > > > > About `StreamThreadNotStartedException`: I understand what you pointed > out. However, I think we can consider the following: If a thread is not > started yet, and `KafkaStreams#store()` throw this exception, we would > not return a `CompositeReadOnlyXxxStore` to the user. Hence, `get()` > cannot be called. And if we return `CompositeReadOnlyXxxStore` the > thread was started and `get()` would never hit the condition to throw > the exception? Or do I miss something (this part of the logic is a > little tricky...) > > However, thinking about it, what could happen IMHO is, that the > corresponding thread crashes after we handed out the store handle. For > this case, it would make sense to throw an exception from `get()` but it > would be a different one IMHO. Maybe we need a new type > (`StreamThreadDeadException` or similar?) or we should reuse > `StoreMigratedException` because if a thread dies we would migrate the > store to another thread. (The tricky part might be, to detect this > condition correctly -- not 100% sure atm how we could do this.) > > What do you think about this? > > > > About `KafkaStreamsNotRunningException` vs > `StreamThreadNotRunningException` -- I see your point. Atm, I think we > don't allow querying at all if KafkaStreams is not in state RUNNING > (correct me if I am wrong). Hence, if there is an instance with 2 > thread, and 1 thread is actually up and ready, but the other thread is > not, you cannot query anything. Only if both threads are in state > RUNNING we allow to query. It might be possible to change the code to > allow querying if a thread is ready independent from the other threads. > For this case, the name you suggest would make more sense. But I > _think_, that the current behavior is different and thus, > `KafkaStreamsNotRunningException` seems to reflect the current behavior > better? -- I also want to add that we are talking about a fatal > exception -- if a thread crashes, we would migrate the store to another > thread and it would not be fatal, but the store can be re-discovered. > Only if all thread would die, if would be fatal -- however, for this > case KafakStreams would transit to DEAD anyway. > > > >> 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()`? >> From my current understanding, I cannot distinguish these two. > > This should be possible. In the private KafkaStreams constructor, we > have access to `InternalTopologyBuilder` that can give us all known > store names. Hence, we can get a set of all known store names, keep them > as a member variable and use in `KafkaStreams#store()` in an initial > check if the store name is valid or not. > > > >> Should we remove `StreamThreadNotRunningException` and throw >> `FatalStateStoreException` directly ? > > I would keep both, because `FatalStateStoreException` is not very > descriptive. Also, we should still have fatal exception > `StateStoreNotAvailableException`? Not sure why you remove it? > > > > Glad you found a way to avoid > `QueryableStoreType#setStreams(KafkaStreams streams)`. > > > > -Matthias > > > On 7/5/19 8:03 AM, Vito Jeng wrote: >> Hi, Mattias, >> >> Just completed the modification of KIP, please take a look when you are >> available. >> >> --- >> Vito >> >> >> On Wed, Jul 3, 2019 at 9:07 PM Vito Jeng <v...@is-land.com.tw> wrote: >> >>> Hi, Matthias, >>> >>> This is second part. >>> >>>> For the internal exceptions: >>>> >>>> `StateStoreClosedException` -- why can it be wrapped as >>>> `StreamThreadNotStartedException` ? It seems that the later would only >>>> be thrown by `KafkaStreams#store()` and thus would be throw directly. >>> >>> Both `StateStoreClosedException` and `EmptyStateStoreException` not can be >>> wrapped as `StreamThreadNotStartedException`. >>> This is a mistaken written in the previous KIP. Thank you point this. >>> >>>> A closed-exception should only happen after a store was successfully >>>> retrieved but cannot be queried any longer? Hence, converting/wrapping >>>> it into a `StateStoreMigratedException` make sense. I am also not sure, >>>> when a closed-exception would be wrapped by a >>>> `StateStoreNotAvailableException` (implying my understanding as describe >>>> above)? >>>> >>>> Same questions about `EmptyStateStoreException`. >>>> >>>> Thinking about both internal exceptions twice, I am wondering if it >>>> makes sense to have both internal exceptions at all? I have the >>>> impression that it make only sense to wrap them with a >>>> `StateStoreMigragedException`, but if they are wrapped into the same >>>> exception all the time, we can just remove both and throw >>>> `StateStoreMigratedException` directly? >>> >>> After deeper thinking, I think you are right. It seems we can throw >>> `StateStoreMigratedException` directly. >>> So that we can remove `StateStoreClosedException`, >>> `EmptyStateStoreException` and `StateStoreNotAvailableException`. >>> Will update the KIP. >>> >>> BTW, if we remove above three exceptions, the >>> `StreamThreadNotRunningException` will be the only one sub class extends >>> from FatalStateStoreException. >>> Should we remove `StreamThreadNotRunningException` and throw >>> `FatalStateStoreException` directly ? >>> >>>> Last point: Why do we need to add? >>>> QueryableStoreType#setStreams(KafkaStreams streams); >>>> John asked this question already and you replied to it. But I am not >>>> sure what your answer means. Can you explain it in more detail? >>> >>> The main purpose is to pass the KafkaStreams reference into >>> CompositeReadOnlyKeyValueStore / CompositeReadOnlySessionStore/ >>> CompositeReadOnlyWindowStore instance. >>> We need check KafkaStreams state to warp InvalidStateStoreException in to >>> other exception(e.g., StateStoreMigratedException) when the user accesses >>> these read-only stores. >>> >>> The original thought is to add `setStreams` method in to >>> QueryableStoreType. But now I think I find a better way during recent days. >>> This way does not need to change any public interface. So we can skip this >>> question. :) >>> >>> >>> I will update the KIP based on our discussion. >>> Thank you for help to finish the KIP! >>> >>> --- >>> Vito >>> >>> >>> On Thu, Jun 6, 2019 at 8:23 AM Matthias J. Sax <matth...@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 <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 >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>> >>>> >>>> >> >
signature.asc
Description: OpenPGP digital signature
