Thanks Vito, for incorporating this. Makes sense. -Navinder
On Wednesday, February 5, 2020, 12:17 AM, Matthias J. Sax <mj...@apache.org> wrote: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 Thanks Vito! That makes sense to me. On 2/1/20 11:29 PM, Vito Jeng wrote: > Hi, folks, > > KIP-562(KAFKA-9445) already merged three days ago. > > I have updated KIP-216 to reflect the KIP-562. The main change is > to introduce a new exception `InvalidStateStorePartitionException`, > will be thrown when user requested partition not available. > > Please take a look and any feedback is welcome. Thanks Matthias for > the reminder. > > --- Vito > > > On Thu, Jan 23, 2020 at 2:13 PM Vito Jeng <v...@is-land.com.tw> > wrote: > >> Got it, thanks Matthias. >> >> --- Vito >> >> >> On Thu, Jan 23, 2020 at 1:31 PM Matthias J. Sax >> <matth...@confluent.io> wrote: >> >>> Thanks Vito. >>> >>> I am also ok with either name. Just a personal slight >>> preference, but not a important. >>> >>> >>> -Matthias >>> >>> On 1/21/20 6:52 PM, Vito Jeng wrote: >>>> Thanks Matthias. >>>> >>>> The KIP is about InvalidStateStoreException. I pick >>>> `StateStoreNotAvailableException` because it may be more >>> intuitive >>>> than `StreamsNotRunningException`. >>>> >>>> No matter which one picked, it's good to me. >>>> >>>> --- Vito >>>> >>>> >>>> On Wed, Jan 22, 2020 at 7:44 AM Matthias J. Sax >>>> <matth...@confluent.io> wrote: >>>> >>>>> Thanks for updating the KIP! >>>>> >>>>> One last comment/question: you kept >>>>> `StateStoreNotAvailableException` >>> in >>>>> favor of `StreamsNotRunningException` (to merge both as >>>>> suggested). >>>>> >>>>> I am wondering, if it might be better to keep >>>>> `StreamsNotRunningException` instead of >>>>> `StateStoreNotAvailableException`, because this exception >>>>> is thrown if Streams is in state PENDING_SHUTDOWN / >>>>> NOT_RUNNING / ERROR ? >>>>> >>>>> >>>>> >>>>> -Matthias >>>>> >>>>> On 1/17/20 9:56 PM, John Roesler wrote: >>>>>> Thanks, Vito. I've just cast my vote. -John >>>>>> >>>>>> On Fri, Jan 17, 2020, at 21:32, Vito Jeng wrote: >>>>>>> Hi, folks, >>>>>>> >>>>>>> Just update the KIP, please take a look. >>>>>>> >>>>>>> Thanks! >>>>>>> >>>>>>> --- Vito >>>>>>> >>>>>>> >>>>>>> On Fri, Jan 17, 2020 at 9:12 AM Vito Jeng >>>>>>> <v...@is-land.com.tw> >>> wrote: >>>>>>> >>>>>>>> Thanks Bill, John and Matthias. Glad you guys joined >>>>>>>> this >>> discussion. >>>>>>>> I got a lot out of the discussion. >>>>>>>> >>>>>>>> I would like to update KIP-216 base on John's >>>>>>>> suggestion to remove >>> the >>>>>>>> category. >>>>>>>> >>>>>>>> >>>>>>>> --- Vito >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jan 17, 2020 at 2:30 AM Matthias J. Sax < >>> matth...@confluent.io >>>>>> >>>>>>>> wrote: >>>>>>>> >>>>>>>>>> Nevertheless, if we omit the categorization, it’s >>>>>>>>>> moot. >>>>>>>>> >>>>>>>>> Ack. >>>>>>>>> >>>>>>>>> I am fine to remove the middle tier. As John >>>>>>>>> pointed out, it might >>> be >>>>>>>>> weird to have only one concrete exception type per >>>>>>>>> category. We can >>>>> also >>>>>>>>> explain in detail how to handle each exception in >>>>>>>>> their JavaDocs. >>>>>>>>> >>>>>>>>> >>>>>>>>> -Matthias >>>>>>>>> >>>>>>>>> On 1/16/20 6:38 AM, Bill Bejeck wrote: >>>>>>>>>> Vito, >>>>>>>>>> >>>>>>>>>> Thanks for the updates, the KIP LGTM. >>>>>>>>>> >>>>>>>>>> -Bill >>>>>>>>>> >>>>>>>>>> On Wed, Jan 15, 2020 at 11:31 PM John Roesler < >>> vvcep...@apache.org> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi Vito, >>>>>>>>>>> >>>>>>>>>>> Haha, your archive game is on point! >>>>>>>>>>> >>>>>>>>>>> What Matthias said in that email is essentially >>>>>>>>>>> what I figured >>> was >>>>> the >>>>>>>>>>> rationale. It makes sense, but the point I was >>>>>>>>>>> making is that >>> this >>>>>>>>> really >>>>>>>>>>> doesn’t seem like a good way to structure a >>>>>>>>>>> production app. On >>> the >>>>>>>>> other >>>>>>>>>>> hand, considering the exception fatal has a >>>>>>>>>>> good chance of >>> avoiding >>>>> a >>>>>>>>>>> frustrating debug session if you just forgot to >>>>>>>>>>> call start. >>>>>>>>>>> >>>>>>>>>>> Nevertheless, if we omit the categorization, >>>>>>>>>>> it’s moot. >>>>>>>>>>> >>>>>>>>>>> It would be easy to add a categorization layer >>>>>>>>>>> later if we want >>> it, >>>>> but >>>>>>>>>>> not very easy to change it if we get it wrong. >>>>>>>>>>> >>>>>>>>>>> Thanks for your consideration! -John >>>>>>>>>>> >>>>>>>>>>> On Wed, Jan 15, 2020, at 21:14, Vito Jeng >>>>>>>>>>> wrote: >>>>>>>>>>>> Hi John, >>>>>>>>>>>> >>>>>>>>>>>> About `StreamsNotStartedException is strange` >>>>>>>>>>>> -- The original idea came from Matthias, two >>>>>>>>>>>> years ago. :) You can reference here: >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>> >>>>> >>> https://mail-archives.apache.org/mod_mbox/kafka-dev/201806.mbox/%3c6 c32083e-b63c-435b-521d-032d45cc5...@confluent.io%3e >>>>>>>>>>>> >>>>>>>>>>>> >>> About omitting the categorization -- >>>>>>>>>>>> It looks reasonable. I'm fine with omitting >>>>>>>>>>>> the categorization >>> but >>>>> not >>>>>>>>>>> very >>>>>>>>>>>> sure it is a good choice. Does any other >>>>>>>>>>>> folks provide opinion? >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi, folks, >>>>>>>>>>>> >>>>>>>>>>>> Just update the KIP-216, please take a look. >>>>>>>>>>>> >>>>>>>>>>>> --- Vito >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Jan 16, 2020 at 6:35 AM Vito Jeng >>>>>>>>>>>> <v...@is-land.com.tw> >>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Hi, folks, >>>>>>>>>>>>> >>>>>>>>>>>>> Thank you suggestion, really appreciate it. >>>>>>>>>>>>> :) I understand your concern. I'll merge >>> StreamsNotRunningException >>>>> and >>>>>>>>>>>>> StateStoreNotAvailableException. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> --- Vito >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Jan 16, 2020 at 6:22 AM John >>>>>>>>>>>>> Roesler < >>> vvcep...@apache.org >>>>>> >>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hey Vito, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Yes, thanks for the KIP. Sorry the >>>>>>>>>>>>>> discussion has been so >>> long. >>>>>>>>>>>>>> Hopefully, we can close it out soon. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I agree we can drop >>>>>>>>>>>>>> StreamsNotRunningException in favor of >>>>>>>>>>>>>> just StateStoreNotAvailableException. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Unfortunately, I have some higher-level >>>>>>>>>>>>>> concerns. The value of these exceptions >>>>>>>>>>>>>> is that they tell you how to handle the >>>>>>>>>>>>>> various situations that can arise while >>>>>>>>>>>>>> querying a distributed data store. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Ideally, as a caller, I should be able to >>>>>>>>>>>>>> just catch >>> "retriable" >>>>> or >>>>>>>>>>>>>> "fatal" and handle them appropriately. >>>>>>>>>>>>>> Otherwise, there's no point in having >>>>>>>>>>>>>> categories, and we should just have all >>>>>>>>>>>>>> the exceptions extend >>>>>>>>>>>>>> InvalidStateStoreException. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Presently, it's not possible to tell from >>>>>>>>>>>>>> just the "retriable"/"fatal" distinction >>>>>>>>>>>>>> what to do. You can tell from the >>>>>>>>>>>>>> descriptions of the various exceptions. >>>>>>>>>>>>>> E.g.: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Retriable: * StreamsRebalancingException: >>>>>>>>>>>>>> the exact same call should just be >>>>>>>>>>>>>> retried until the rebalance is complete * >>>>>>>>>>>>>> StateStoreMigratedException: the store >>>>>>>>>>>>>> handle is now invalid, so you need to >>>>>>>>>>>>>> re-discover the instance and get a new >>>>>>>>>>>>>> handle on that instance. In other words, >>>>>>>>>>>>>> the query itself may be valid, but the >>>>>>>>>>>>>> particular method invocation on this >>>>>>>>>>>>>> particular instance has encountered a >>>>>>>>>>>>>> fatal exception. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Fatal: * UnknownStateStoreException: this >>>>>>>>>>>>>> is truly fatal. No amount of retrying or >>>>>>>>>>>>>> re-discovering is going to get you a >>>>>>>>>>>>>> handle >>>>> on a >>>>>>>>>>>>>> store that doesn't exist in the cluster. >>>>>>>>>>>>>> * StateStoreNotAvailableException: this >>>>>>>>>>>>>> is actually >>> recoverable, >>>>>>>>>>>>>> since the store might exist in the >>>>>>>>>>>>>> cluster, but isn't >>>>> available >>>>>>>>> on >>>>>>>>>>>>>> this particular instance (which is shut >>>>>>>>>>>>>> down or whatever). >>>>>>>>>>>>>> >>>>>>>>>>>>>> Personally, I'm not a fan of code >>>>>>>>>>>>>> bureaucracy, so I'm 100% >>> fine >>>>>>>>>>>>>> with omitting the categorization and just >>>>>>>>>>>>>> having 5 subclasses of >>>>>>>>>>>>>> InvalidStateStoreException. Each of them >>>>>>>>>>>>>> would tell you how to handle them, and >>>>>>>>>>>>>> it's not too many to really understand >>>>>>>>>>>>>> and handle each one. >>>>>>>>>>>>>> >>>>>>>>>>>>>> If you really want to have a middle tier, >>>>>>>>>>>>>> I'd recommend: * >>>>>>>>>>>>>> RetryableStateStoreException: the exact >>>>>>>>>>>>>> same call should be repeated. * >>>>>>>>>>>>>> RecoverableStateStoreException: the store >>>>>>>>>>>>>> handle should be discarded and the caller >>>>>>>>>>>>>> should re-discover the location of the >>>>>>>>>>>>>> store and repeat the query on the correct >>>>>>>>>>>>>> instance. * FatalStateStoreException: the >>>>>>>>>>>>>> query/request is totally invalid and will >>>>>>>>>>>>>> never succeed. >>>>>>>>>>>>>> >>>>>>>>>>>>>> However, attempting to categorize the >>>>>>>>>>>>>> proposed exceptions reveals even problems >>>>>>>>>>>>>> with this categorization: Retriable: * >>>>>>>>>>>>>> StreamsRebalancingException Recoverable: >>>>>>>>>>>>>> * StateStoreMigratedException * >>>>>>>>>>>>>> StreamsNotRunningException Fatal: * >>>>>>>>>>>>>> UnknownStateStoreException >>>>>>>>>>>>>> >>>>>>>>>>>>>> But StreamsNotStartedException is >>>>>>>>>>>>>> strange... It means that one code path >>>>>>>>>>>>>> got a handle on a specific KafkaStreams >>>>>>>>>>>>>> object instance and sent it a query >>>>>>>>>>>>>> before another code path invoked the >>>>>>>>>>>>>> start() method on the exact same object >>>>>>>>>>>>>> instance. It seems like the most likely >>>>>>>>>>>>>> scenario is that whoever wrote the >>>>>>>>>>>>>> program just forgot to call start() >>>>>>>>>>>>>> before querying, in which case, retrying >>>>>>>>>>>>>> isn't going to help, and a fatal >>> exception >>>>>>>>>>>>>> is more appropriate. I.e., it sounds like >>>>>>>>>>>>>> a "first 15 minutes experience" problem, >>>>>>>>>>>>>> and making it fatal would be more >>>>>>>>>>>>>> helpful. Even in a production context, >>>>>>>>>>>>>> there's no reason not to sequence your >>>>>>>>>>>>>> application startup such that you don't >>>>>>>>>>>>>> accept queries until after Streams is >>>>>>>>>>>>>> started. Thus, I guess I'd categorize it >>>>>>>>>>>>>> under "fatal". >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regardless of whether you make it fatal >>>>>>>>>>>>>> or retriable, you'd still have a whole >>>>>>>>>>>>>> category with only one exception in it, >>>>>>>>>>>>>> and the other two categories only have >>>>>>>>>>>>>> two exceptions. Plus, as you pointed out >>>>>>>>>>>>>> in the KIP, you can't get all exceptions >>>>>>>>>>>>>> in all cases anyway: * store() can only >>>>>>>>>>>>>> throw NotStarted, NotRunning, and >>>>>>>>>>>>>> Unknown * actual store queries can only >>>>>>>>>>>>>> throw Rebalancing, Migrated, and >>>>>>>>>>>>>> NotRunning >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thus, in practice also, there are exactly >>>>>>>>>>>>>> three categories and also exactly three >>>>>>>>>>>>>> exception types. It doesn't seem like >>>>>>>>>>>>>> there's a great advantage to the >>>>>>>>>>>>>> categories here. To avoid the >>>>>>>>>>>>>> categorization problem and also to >>>>>>>>>>>>>> clarify what exceptions can actually be >>>>>>>>>>>>>> thrown in different circumstances, it >>>>>>>>>>>>>> seems like we should just: * get rid of >>>>>>>>>>>>>> the middle tier and make all the >>>>>>>>>>>>>> exceptions extend >>>>>>>>>>>>>> InvalidStateStoreException * drop >>>>>>>>>>>>>> StateStoreNotAvailableException in favor >>>>>>>>>>>>>> of StreamsNotRunningException * clearly >>>>>>>>>>>>>> document on all public methods which >>>>>>>>>>>>>> exceptions need to be handled >>>>>>>>>>>>>> >>>>>>>>>>>>>> How do you feel about this? Thanks, >>>>>>>>>>>>>> -John >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Wed, Jan 15, 2020, at 15:13, Bill >>>>>>>>>>>>>> Bejeck wrote: >>>>>>>>>>>>>>> Thanks for KIP Vito. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Overall the KIP LGTM, but I'd have to >>>>>>>>>>>>>>> agree with others on >>>>> merging >>>>>>>>>>> the >>>>>>>>>>>>>>> `StreamsNotRunningException` and >>>>> `StateStoreNotAvailableException` >>>>>>>>>>>>>> classes. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Since in both cases, the thread state >>>>>>>>>>>>>>> is in >>> `PENDING_SHUTDOWN || >>>>>>>>>>>>>>> NOT_RUNNING || ERROR` I'm not even sure >>>>>>>>>>>>>>> how we could >>> distinguish >>>>>>>>>>> when to >>>>>>>>>>>>>>> use the different exceptions. Maybe a >>>>>>>>>>>>>>> good middle ground would be to have a >>>>> detailed >>>>>>>>>>>>>>> exception message. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The KIP freeze is close, so I think if >>>>>>>>>>>>>>> we can agree on this, >>> we >>>>> can >>>>>>>>>>>>>> wrap up >>>>>>>>>>>>>>> the voting soon. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks, Bill >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Tue, Jan 14, 2020 at 2:12 PM >>>>>>>>>>>>>>> Matthias J. Sax < >>>>>>>>>>> matth...@confluent.io> >>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Vito, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> It's still unclear to me what the >>>>>>>>>>>>>>>> advantage is, to have both >>>>>>>>>>>>>>>> `StreamsNotRunningException` and >>>>>>>>>>> `StateStoreNotAvailableException`? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> For both cased, the state is >>>>>>>>>>>>>>>> `PENDING_SHUTDOWN / >>> NOT_RUNNING / >>>>>>>>>>> ERROR` >>>>>>>>>>>>>>>> and thus, for a user point of view, >>>>>>>>>>>>>>>> why does it matter if >>> the >>>>>>>>>>> store is >>>>>>>>>>>>>>>> closed on not? I don't understand >>>>>>>>>>>>>>>> why/how this information >>>>> would >>>>>>>>>>> be >>>>>>>>>>>>>>>> useful? Do you have a concrete >>>>>>>>>>>>>>>> example in mind how a user >>> would >>>>>>>>>>> react >>>>>>>>>>>>>>>> differently to both exceptions? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> @Vinoth: about >>>>>>>>>>>>>>>> `StreamsRebalancingException` -- to >>>>>>>>>>>>>>>> me, it >>> seems >>>>>>>>>>> best >>>>>>>>>>>>>> to >>>>>>>>>>>>>>>> actually do this on a per-query >>>>>>>>>>>>>>>> basis, ie, have an overload >>>>>>>>>>>>>>>> `KafkaStreams#store(...)` that takes >>>>>>>>>>>>>>>> a boolean flag that >>> allow >>>>> to >>>>>>>>>>>>>>>> _disable_ the exception and opt-in to >>>>>>>>>>>>>>>> query a active store >>>>> during >>>>>>>>>>>>>>>> recovery. However, as KIP-535 >>>>>>>>>>>>>>>> actually introduces this >>> change >>>>> in >>>>>>>>>>>>>>>> behavior, I think KIP-216 should not >>>>>>>>>>>>>>>> cover this, but KIP-535 >>>>>>>>>>> should be >>>>>>>>>>>>>>>> updated. I'll follow up on the other >>>>>>>>>>>>>>>> KIP thread to raise >>> this >>>>>>>>>>> point. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 1/11/20 12:26 AM, Vito Jeng >>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>> Hi, Matthias & Vinoth, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Thanks for the feedback. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> What is still unclear to me is, >>>>>>>>>>>>>>>>>> what we gain by having >>> both >>>>>>>>>>>>>>>>>> `StreamsNotRunningException` and >>>>>>>>>>>>>> `StateStoreNotAvailableException`. Both >>>>>>>>>>>>>>>>>> exception are thrown when >>>>>>>>>>>>>>>>>> KafkaStreams is in state >>>>>>>>>>>>>> PENDING_SHUTDOWN / >>>>>>>>>>>>>>>>>> NOT_RUNNING / ERROR. Hence, as a >>>>>>>>>>>>>>>>>> user what do I gain to >>> know >>>>>>>>>>> if the >>>>>>>>>>>>>>>>>> state store is closed on not -- I >>>>>>>>>>>>>>>>>> can't query it anyway? >>>>> Maybe >>>>>>>>>>> I >>>>>>>>>>>>>> miss >>>>>>>>>>>>>>>>>> something thought? >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Yes, both >>>>>>>>>>>>>>>>> `StreamsNotRunningException` and >>>>>>>>>>>>>>>>> `StateStoreNotAvailableException` >>>>>>>>>>>>>>>>> are fatal exception. But >>>>>>>>>>>>>>>>> `StateStoreNotAvailableException` >>>>>>>>>>>>>>>>> is fatal exception >>> about >>>>>>>>>>> state >>>>>>>>>>>>>>>> store >>>>>>>>>>>>>>>>> related. I think it would be >>>>>>>>>>>>>>>>> helpful that if user need to >>> distinguish >>>>>>>>>>> these >>>>>>>>>>>>>> two >>>>>>>>>>>>>>>>> different case to handle it. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I'm not very sure, does that make >>>>>>>>>>>>>>>>> sense? >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> --- Vito >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Fri, Jan 10, 2020 at 3:35 AM >>>>>>>>>>>>>>>>> Vinoth Chandar < >>>>>>>>>>> vin...@apache.org> >>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> +1 on merging >>>>>>>>>>>>>>>>>> `StreamsNotRunningException` and >>>>>>>>>>>>>>>>>> `StateStoreNotAvailableException`, >>>>>>>>>>>>>>>>>> both exceptions are >>> fatal >>>>>>>>>>>>>> anyway. IMO >>>>>>>>>>>>>>>>>> its best to have these exceptions >>>>>>>>>>>>>>>>>> be about the state store >>>>>>>>>>> (and not >>>>>>>>>>>>>>>> streams >>>>>>>>>>>>>>>>>> state), to easier understanding. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Additionally, KIP-535 allows for >>>>>>>>>>>>>>>>>> querying of state stores >>> in >>>>>>>>>>>>>> rebalancing >>>>>>>>>>>>>>>>>> state. So do we need the >>>>>>>>>>>>>>>>>> StreamsRebalancingException? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 2020/01/09 03:38:11, "Matthias >>>>>>>>>>>>>>>>>> J. Sax" < >>>>>>>>>>> matth...@confluent.io> >>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>> Sorry that I dropped the ball >>>>>>>>>>>>>>>>>>> on this... >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks for updating the KIP. >>>>>>>>>>>>>>>>>>> Overall LGTM now. Feel free >>> to >>>>>>>>>>> start >>>>>>>>>>>>>> a >>>>>>>>>>>>>>>> VOTE >>>>>>>>>>>>>>>>>>> thread. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> What is still unclear to me is, >>>>>>>>>>>>>>>>>>> what we gain by having >>> both >>>>>>>>>>>>>>>>>>> `StreamsNotRunningException` >>>>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>> `StateStoreNotAvailableException`. >>>>>>>>>>>>>>>> Both >>>>>>>>>>>>>>>>>>> exception are thrown when >>>>>>>>>>>>>>>>>>> KafkaStreams is in state >>>>>>>>>>>>>> PENDING_SHUTDOWN / >>>>>>>>>>>>>>>>>>> NOT_RUNNING / ERROR. Hence, as >>>>>>>>>>>>>>>>>>> a user what do I gain to >>> know >>>>>>>>>>> if >>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>> state store is closed on not -- >>>>>>>>>>>>>>>>>>> I can't query it anyway? >>>>>>>>>>> Maybe I >>>>>>>>>>>>>> miss >>>>>>>>>>>>>>>>>>> something thought? >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> On 11/3/19 6:07 PM, Vito Jeng >>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>> Sorry for the late reply, >>>>>>>>>>>>>>>>>>>> thanks for the review. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> About >>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Why is it only thrown if the state is REBALANCING? A >>> store >>>>>>>>>>>>>> might be >>>>>>>>>>>>>>>>>>>>> migrated during a >>>>>>>>>>>>>>>>>>>>> rebalance, and Kafka >>>>>>>>>>>>>>>>>>>>> Streams might >>>>> resume >>>>>>>>>>>>>> back to >>>>>>>>>>>>>>>>>>>>> RUNNING state and afterward >>>>>>>>>>>>>>>>>>>>> somebody tries to use an >>> old >>>>>>>>>>> store >>>>>>>>>>>>>>>> handle. >>>>>>>>>>>>>>>>>>>>> Also, if state is >>>>>>>>>>>>>>>>>>>>> REBALANCING, should we >>>>>>>>>>>>>>>>>>>>> throw >>>>>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException`? >>>>>>>>>>>>>>>>>>>>> Hence, I think >>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException` >>>>>>>>>>>>>>>>>>>>> does only make sense >>> during >>>>>>>>>>>>>> `RUNNING` >>>>>>>>>>>>>>>>>> state. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Thank you point this, already >>>>>>>>>>>>>>>>>>>> updated. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Why do we need to distinguish >>>>>>>>>>>>>>>>>>>> between >>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` >>>>>>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>> `StateStoreNotAvailableException`? >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` may be caused by >>> various >>>>>>>>>>>>>> reasons, I >>>>>>>>>>>>>>>>>> think >>>>>>>>>>>>>>>>>>>> it would be helpful that the >>>>>>>>>>>>>>>>>>>> user can distinguish whether >>>>>>>>>>>>>>>>>>>> it is caused by the state >>>>> store >>>>>>>>>>>>>> closed. >>>>>>>>>>>>>>>>>>>> (Maybe I am wrong...) >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Last, why do we distinguish >>>>>>>>>>>>>>>>>>>> between `KafkaStreams` >>> instance >>>>>>>>>>> and >>>>>>>>>>>>>>>>>>>>> `StreamsThread`? To me, it >>>>>>>>>>>>>>>>>>>>> seems we should always >>> refer to >>>>>>>>>>> the >>>>>>>>>>>>>>>>>> instance, >>>>>>>>>>>>>>>>>>>>> because that is the level >>>>>>>>>>>>>>>>>>>>> of granularity in which we >>>>>>>>>>>>>> enable/disable >>>>>>>>>>>>>>>>>> IQ atm. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Totally agree. Do you mean >>>>>>>>>>>>>>>>>>>> the naming of state store >>>>>>>>>>> exceptions? >>>>>>>>>>>>>>>>>>>> I don't have special reason >>>>>>>>>>>>>>>>>>>> to distinguish these two. >>>>>>>>>>>>>>>>>>>> Your suggestion look more >>>>>>>>>>>>>>>>>>>> reasonable for the exception >>>>>>>>>>> naming. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Last, for >>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`, >>>>>>>>>>>>>>>>>>>> I would add >>> that a >>>>>>>>>>> user >>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>> rediscover the store and >>>>>>>>>>>>>>>>>>>>> cannot blindly retry as >>>>>>>>>>>>>>>>>>>>> the >>> store >>>>>>>>>>>>>> handle is >>>>>>>>>>>>>>>>>>>>> invalid and a new store >>>>>>>>>>>>>>>>>>>>> handle must be retrieved. >>>>>>>>>>>>>>>>>>>>> That >>> is >>>>> a >>>>>>>>>>>>>>>> difference >>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException` >>>>>>>>>>>>>>>>>>>>> that allows for >>>>>>>>>>> "blind" >>>>>>>>>>>>>> retries >>>>>>>>>>>>>>>>>>>>> that either resolve (if the >>>>>>>>>>>>>>>>>>>>> store is still on the same >>>>>>>>>>> instance >>>>>>>>>>>>>> after >>>>>>>>>>>>>>>>>>>>> rebalancing finishes, or >>>>>>>>>>>>>>>>>>>>> changes to >>>>>>>>>>>>>> `StateStoreMigratedException` if >>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>> store was migrated away >>>>>>>>>>>>>>>>>>>>> during rebalancing). >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Nice, it's great! Thank you. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> The KIP already updated, >>>>>>>>>>>>>>>>>>>> please take a look. :) >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On Wed, Oct 23, 2019 at 1:48 >>>>>>>>>>>>>>>>>>>> PM Matthias J. Sax < >>>>>>>>>>>>>>>> matth...@confluent.io >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Any update on this KIP? >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On 10/7/19 3:35 PM, >>>>>>>>>>>>>>>>>>>>> Matthias J. Sax wrote: >>>>>>>>>>>>>>>>>>>>>> Sorry for the late reply. >>>>>>>>>>>>>>>>>>>>>> The 2.4 deadline kept us >>> quite >>>>>>>>>>> busy. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> About >>>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`: >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Why is it only thrown if the state is REBALANCING? A >>>>> store >>>>>>>>>>>>>> might be >>>>>>>>>>>>>>>>>>>>>> migrated during a >>>>>>>>>>>>>>>>>>>>>> rebalance, and Kafka >>>>>>>>>>>>>>>>>>>>>> Streams might >>>>> resume >>>>>>>>>>>>>> back to >>>>>>>>>>>>>>>>>>>>>> RUNNING state and >>>>>>>>>>>>>>>>>>>>>> afterward somebody tries >>>>>>>>>>>>>>>>>>>>>> to use an >>> old >>>>>>>>>>> store >>>>>>>>>>>>>>>>>> handle. >>>>>>>>>>>>>>>>>>>>>> Also, if state is >>>>>>>>>>>>>>>>>>>>>> REBALANCING, should we >>>>>>>>>>>>>>>>>>>>>> throw >>>>>>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException`? >>>>>>>>>>>>>>>>>>>>>> Hence, I think >>>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException` >>>>>>>>>>>>>>>>>>>>>> does only make sense >>> during >>>>>>>>>>>>>> `RUNNING` >>>>>>>>>>>>>>>>>>>>> state. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Why do we need to >>>>>>>>>>>>>>>>>>>>>> distinguish between >>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> and `StateStoreNotAvailableException`? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Last, why do we >>>>>>>>>>>>>>>>>>>>>> distinguish between >>>>>>>>>>>>>>>>>>>>>> `KafkaStreams` >>>>>>>>>>> instance and >>>>>>>>>>>>>>>>>>>>>> `StreamsThread`? To me, >>>>>>>>>>>>>>>>>>>>>> it seems we should >>>>>>>>>>>>>>>>>>>>>> always >>> refer >>>>> to >>>>>>>>>>> the >>>>>>>>>>>>>>>>>> instance, >>>>>>>>>>>>>>>>>>>>>> because that is the level >>>>>>>>>>>>>>>>>>>>>> of granularity in which >>>>>>>>>>>>>>>>>>>>>> we >>>>>>>>>>>>>> enable/disable >>>>>>>>>>>>>>>>>> IQ >>>>>>>>>>>>>>>>>>>>> atm. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Last, for >>>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`, >>>>>>>>>>>>>>>>>>>>>> I would add >>>>> that a >>>>>>>>>>>>>> user >>>>>>>>>>>>>>>>>> need to >>>>>>>>>>>>>>>>>>>>>> rediscover the store and >>>>>>>>>>>>>>>>>>>>>> cannot blindly retry as >>>>>>>>>>>>>>>>>>>>>> the >>>>> store >>>>>>>>>>>>>> handle is >>>>>>>>>>>>>>>>>>>>>> invalid and a new store >>>>>>>>>>>>>>>>>>>>>> handle must be retrieved. >>>>>>>>>>>>>>>>>>>>>> That >>>>> is a >>>>>>>>>>>>>>>>>> difference >>>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>> `StreamThreadRebalancingException` >>>>>>>>>>>>>>>>>>>>>> that allows for >>>>>>>>>>> "blind" >>>>>>>>>>>>>>>> retries >>>>>>>>>>>>>>>>>>>>>> that either resolve (if >>>>>>>>>>>>>>>>>>>>>> the store is still on the >>>>>>>>>>>>>>>>>>>>>> same >>>>>>>>>>> instance >>>>>>>>>>>>>>>> after >>>>>>>>>>>>>>>>>>>>>> rebalancing finishes, or >>>>>>>>>>>>>>>>>>>>>> changes to >>>>>>>>>>>>>> `StateStoreMigratedException` if >>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>> store was migrated away >>>>>>>>>>>>>>>>>>>>>> during rebalancing). >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> -Matthias >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> On 8/9/19 10:20 AM, Vito >>>>>>>>>>>>>>>>>>>>>> Jeng wrote: >>>>>>>>>>>>>>>>>>>>>>> My bad. The short link >>>>>>>>>>>>>>>>>>>>>>> `https://shorturl.at/CDNT9` >>> >>>>>>>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>> <https://shorturl.at/CDNT9> >>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> <https://shorturl.at/CDNT9> seems incorrect. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> Please use the >>>>>>>>>>>>>>>>>>>>>>> following instead: >>>>>>>>>>> https://shorturl.at/bkKQU >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> --- Vito >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> On Fri, Aug 9, 2019 at >>>>>>>>>>>>>>>>>>>>>>> 10:53 AM Vito Jeng < >>>>>>>>>>>>>> v...@is-land.com.tw> >>>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> Thanks, Matthias! >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> About >>>>>>>>>>>>>>>>>>>>>>>>> `StreamThreadNotStartedException`: >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> Thank you for explanation. I agree with your >>> opinion. >>>>>>>>>>>>>>>>>>>>>>>> `CompositeReadOnlyXxxStore#get()` >>>>>>>>>>>>>>>>>>>>>>>> would never throw >>>>>>>>>>>>>>>>>>>>>>>> `StreamThreadNotStartedException`. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> For the case that corresponding thread crashes >>> after we >>>>>>>>>>>>>> handed out >>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>>>> store handle. We may >>>>>>>>>>>>>>>>>>>>>>>> throw >>>>>>>>>>> `KafkaStreamsNotRunningException` >>>>>>>>>>>>>> or >>>>>>>>>>>>>>>>>>>>>>>> `StateStoreMigratedException`. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> In `StreamThreadStateStoreProvider`, we would throw >>>>>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` >>>>>>>>>>>>>>>>>>>>>>>> when stream >>> thread is >>>>>>>>>>> not >>>>>>>>>>>>>>>>>> running( >>>>>>>>>>>>>>>>>>>>>>>> https://shorturl.at/CDNT9) >>>>>>>>>>>>>>>>>>>>>>>> or throw >>>>>>>>>>>>>> `StateStoreMigratedException` >>>>>>>>>>>>>>>>>> when >>>>>>>>>>>>>>>>>>>>>>>> store is >>>>>>>>>>>>>>>>>>>>>>>> closed(https://shorturl.at/hrvAN). >>>>>>>>>>>>>>>>>>>>>>>> So I >>> think >>>>>>>>>>> we >>>>>>>>>>>>>> do not >>>>>>>>>>>>>>>>>> need >>>>>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>> add a new type for >>>>>>>>>>>>>>>>>>>>>>>> this case. Does that >>>>>>>>>>>>>>>>>>>>>>>> make sense? >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> About >>>>>>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException` >>>>>>>>>>>>>>>>>>>>>>>>> vs >>>>>>>>>>>>>>>>>>>>>>>> `StreamThreadNotRunningException`: >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> I understand your point. I rename >>>>>>>>>>>>>>>>>> `StreamThreadNotRunningException` >>>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>>>>>>>>>> `KafkaStreamsNotRunningException`. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> About check unknown state store names: >>>>>>>>>>>>>>>>>>>>>>>> Thank you for the >>>>>>>>>>>>>>>>>>>>>>>> hint. I add a new >>>>>>>>>>>>>>>>>>>>>>>> type >>>>>>>>>>>>>>>>>> `UnknownStateStoreException` >>>>>>>>>>>>>>>>>>>>> for >>>>>>>>>>>>>>>>>>>>>>>> this case. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> Also, we should >>>>>>>>>>>>>>>>>>>>>>>>> still have fatal >>>>>>>>>>>>>>>>>>>>>>>>> exception >>>>>>>>>>>>>>>>>>>>>>>> `StateStoreNotAvailableException`? >>>>>>>>>>>>>>>>>>>>>>>> Not sure why you >>>>>>>>>>> remove >>>>>>>>>>>>>> it? >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> Thank you point this, >>>>>>>>>>>>>>>>>>>>>>>> already add it >>>>>>>>>>>>>>>>>>>>>>>> again. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> The KIP already >>>>>>>>>>>>>>>>>>>>>>>> updated, please take >>>>>>>>>>>>>>>>>>>>>>>> a look. >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> --- Vito >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>> >>>>> >>>>> >>>> >>> >>> > -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEI8mthP+5zxXZZdDSO4miYXKq/OgFAl45vCwACgkQO4miYXKq /OhT4Q//elvNj2BRIMd7SqyQhpa6sSq4k2iG3wFlr/B2xkmkZPWUICt3SpCisIxG RlP5ml7Mi8IcWv9jmMux9C1NpftboLudxIUcun/I4cP0T3S7ytXfzdO+V1WuD9xe hfeqMwJTytusEv5VO3MyY+jYGqmPJrPJRViZ2Uwsj3Ojk0SZRb/m+b6ah6BGMh0E nlUIux7cabg1OZ/ee0x21hcAB32lVIRfPLKZeHCTHuYmlW76X4pXhFxOm69Pumtj ciVMr/pl7B+XiMk0C2Po04zQcP8+5/O7LYU4e8ha12NtuNuDTlQLGUb+S9Qrbxb2 xdFrlC81RROmIdIOTLiJeeBqhzd19llDgMhBf4spJMOeLn7359PX6r/9tDqonJrQ wpSp4S54MDYfTPHUPqA6u/FsE0BX4EMU71ckC9rXVRRUnQ8A8cMk/6qWlRu8NvxQ IiYuSo48UENlcTHxXC/rzIDp54gvtv2iml8QcOg6tS+hzhAI4yoY+1w4pnjPmRo0 4D1hnq5bd1SLrGSGcCxVbN0jtwgkcr50HBb1UkIDRndqapfKwZOMV65tIwTvxaaZ r2QPYyPd6ZDgeMdy0r94z0SMwatqGCmJD9EQmSmLBulemrvYxPGUevQls+WgE4WE zc3LtoyfdlXlkttTU0Q/dm2H9OU4s3QO+6PEdC+Qwi3aCKKhdUQ= =4EMC -----END PGP SIGNATURE-----
