Overall LGTM. A few minor comments:
> The SHUTDOWN_CLIENT option in the Streams Uncaught Exception Handler should > leave the client state in ERROR instead of NOT_RUNNING and > In order to be consistent, SHUTDOWN_CLIENT will leave the client state in > ERROR instead of NOT_RUNNING Both should also apply to SHUTDOWN_APPLICATION? If not, why? > Close() called on ERROR or PENDING_ERROR will be idempotent Should we replae `idempotent` with `a no-op`, because the difference is that `close()` would normally transit to NOT_RUNNING? The Jira links to 3 tickets, but uses different markup. That is confusing. Also, I actually believe that 6520 is unrelated? -Matthias On 12/10/20 1:52 AM, Bruno Cadonna wrote: > Thanks for the KIP, Walker! > > The KIP looks good to me. I have just a minor comment about the KIP > document. > > You talk about SHUTDOWN_CLIENT in the KIP, but never explain that it is > a possible action that can be taken in the Streams uncaught exception > handler. Could you please clarify that? > > Best, > Bruno > > On 09.12.20 19:04, Walker Carlson wrote: >> Thanks for the comments. If there are no further concerns I would like to >> call for a vote on KIP-696 to clarify and clean up the Streams State >> Machine. >> >> walker >> >> On Wed, Dec 9, 2020 at 8:50 AM John Roesler <vvcep...@apache.org> wrote: >> >>> Thanks, Walker! >>> >>> Your proposal looks good to me. >>> >>> -John >>> >>> On Tue, 2020-12-08 at 18:29 -0800, Walker Carlson wrote: >>>> Thanks for the feedback Guozhang! >>>> >>>> I clarified some of the points in the Proposed Changes section so >>> hopefully >>>> it will be more clear what is going on now. I also agree with your >>>> suggestion about the possible call to close() on ERROR so I added this >>>> line. >>>> "Close() called on ERROR will be idempotent and not throw an exception, >>> but >>>> we will log a warning." >>>> >>>> I have linked those tickets and I will leave a comment trying to >>>> explain >>>> how these changes will affect their issue. >>>> >>>> walker >>>> >>>> On Tue, Dec 8, 2020 at 4:57 PM Guozhang Wang <wangg...@gmail.com> >>>> wrote: >>>> >>>>> Hello Walker, >>>>> >>>>> Thanks for the KIP! Overall it looks reasonable to me. Just a few >>>>> minor >>>>> comments for the wiki page itself: >>>>> >>>>> 1) Could you clarify the conditions when RUNNING / REBALANCING -> >>>>> PENDING_ERROR will happen; and when PENDING_ERROR -> ERROR will >>>>> happen. >>>>> E.g. when I read "Streams will only reach ERROR state in the event of >>> an >>>>> exceptional failure in which the `StreamsUncaughtExceptionHandler` >>> chose to >>>>> either shutdown the application or the client." I thought the first >>>>> transition would happen before the handler, and the second transition >>> would >>>>> happen immediately after the handler returns "shutdown client" or >>> "shutdown >>>>> application", until I read the last statement regarding >>> "SHUTDOWN_CLIENT". >>>>> >>>>> 2) A compatibility issue: today it is possible that users would call >>>>> Streams APIs like shutdown in the global state transition listener. >>>>> And >>>>> it's common to try shutting down the application automatically when >>>>> transiting to ERROR (assuming it was not a terminating state). I think >>> we >>>>> could consider making this call a no-op and log a warning. >>>>> >>>>> 3) Could you link the following JIRAs in the "JIRA" field? >>>>> >>>>> https://issues.apache.org/jira/browse/KAFKA-10555 >>>>> https://issues.apache.org/jira/browse/KAFKA-9638 >>>>> https://issues.apache.org/jira/browse/KAFKA-6520 >>>>> >>>>> And maybe we can also left a comment on those tickets explaining what >>> would >>>>> happen to tackle the issues after this KIP. >>>>> >>>>> >>>>> Guozhang >>>>> >>>>> >>>>> On Tue, Dec 8, 2020 at 12:16 PM Walker Carlson <wcarl...@confluent.io> >>>>> wrote: >>>>> >>>>>> Hello all, >>>>>> >>>>>> I'd like to propose KIP-696 to clarify the meaning of ERROR state in >>> the >>>>>> KafkaStreams Client State Machine. This will update the States to be >>>>>> consistent with changes in KIP-671 and KIP-663. >>>>>> >>>>>> Here are the details: https://cwiki.apache.org/confluence/x/lCvZCQ >>>>>> >>>>>> Thanks, >>>>>> Walker >>>>>> >>>>> >>>>> >>>>> -- >>>>> -- Guozhang >>>>> >>> >>> >>> >>
