Thanks for the comets Matthias. I respond inline below.
Thanks, walker On Sat, Dec 19, 2020 at 11:35 AM Matthias J. Sax <mj...@apache.org> wrote: > 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? > I totally agree. This is already the behavior of SHUTDOWN_APPLICATION we are just bringing SHUTDOWN_CLIENT to match. > > > > 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? > That is fine with me. > > The Jira links to 3 tickets, but uses different markup. That is > confusing. Also, I actually believe that 6520 is unrelated? > I'll fix the mark up, I think that this kip changes how that 6520 is handled but it probably doesn't need to be on the list. I have added a comment on the ticket to notify anyone that related behavior has changed > > > -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 > >>>>> > >>> > >>> > >>> > >> >
