Alright, I made some changes. Matthias, if you had time, it would be good if you made another pass. This should be close to completion.
Cheers, Richard On Sat, Apr 27, 2019 at 3:46 PM Richard Yu <yohan.richard...@gmail.com> wrote: > Hi Matthias, > > Sure, I could do the DISCONNECTED state. > > > On Sat, Apr 27, 2019 at 3:16 PM Matthias J. Sax <matth...@confluent.io> > wrote: > >> Thanks for updating the KIP. I also had a quick look into your PR. >> >> I actually think that the original idea to add a new state DISCONNECTED >> would provide a better user experience. >> >> Your current proposal does not add a new state, even if it mentions this >> in the beginning. Compare: >> >> https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamThread.java#L74-L153 >> >> >> -Matthias >> >> On 4/23/19 7:56 PM, Richard Yu wrote: >> > Oh, so if possible. I thought it would be good if we could finish this >> KIP >> > up. >> > Matthias, or Michael, if you have any further comments, please let me >> know. >> > :) >> > >> > Otherwise, I might restart the voting thread in a few days. >> > >> > Cheers, >> > Richard >> > >> > On Wed, Apr 17, 2019 at 2:30 PM Richard Yu <yohan.richard...@gmail.com> >> > wrote: >> > >> >> Alright, so I made a few changes to the KIP. >> >> I realized that there might be an easier way to give the user >> information >> >> on the connection state of Kafka Streams. >> >> In implementation, if one wishes to have DISCONNECTED as a state, then >> one >> >> would have to factor in proper state transitions. >> >> The other approach that is now outlined in the KIP. Instead, we could >> just >> >> add a method which I think achieves the same effect. >> >> If any of you thinks there is wrong with this approach, please let me >> >> know. :) >> >> >> >> Cheers, >> >> Richard >> >> >> >> On Wed, Apr 17, 2019 at 11:49 AM Richard Yu < >> yohan.richard...@gmail.com> >> >> wrote: >> >> >> >>> I just realized something. >> >>> >> >>> Hi Matthias, might need your input here. >> >>> I realized that when implementing this change, as noted in the JIRA, >> we >> >>> would need to "check the behaviour of the consumer" since its >> consumer's >> >>> connection with broker that we are dealing with. >> >>> >> >>> So doesn't that mean we would also be dealing with consumer API >> changes >> >>> as well? >> >>> I don't think consumer has any methods which would give us the state >> of a >> >>> connection either. >> >>> >> >>> - Richard >> >>> >> >>> On Wed, Apr 17, 2019 at 8:43 AM Richard Yu < >> yohan.richard...@gmail.com> >> >>> wrote: >> >>> >> >>>> Hi Micheal, >> >>>> >> >>>> Yeah, those are some points I should've clarified. >> >>>> No problem. Have got it done. >> >>>> >> >>>> >> >>>> >> >>>> On Wed, Apr 17, 2019 at 6:42 AM Michael Noll <mich...@confluent.io> >> >>>> wrote: >> >>>> >> >>>>> Richard, >> >>>>> >> >>>>> thanks for looking into this! >> >>>>> >> >>>>> However, I have some concerns. The KIP you created ( >> >>>>> >> >>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-457%3A+Add+DISCONNECTED+status+to+Kafka+Streams >> >>>>> ) >> >>>>> doesn't yet address open questions such as the ones mentioned by >> >>>>> Matthias: >> >>>>> >> >>>>> 1) What is the difference between DEAD and the proposed >> DISCONNECTED? >> >>>>> This >> >>>>> should be defined in the KIP. >> >>>>> >> >>>>> 2) Difference between your KIP and the JIRA ( >> >>>>> https://issues.apache.org/jira/browse/KAFKA-6520): In the JIRA >> ticket, >> >>>>> the >> >>>>> DISCONNECTED state was proposed for the scenario that the KStreams >> >>>>> application is healthy but the Kafka broker is down. This is >> different >> >>>>> to >> >>>>> what you wrote in the KIP: "When something happens in Kafka Streams, >> >>>>> such >> >>>>> as an unexpected crash or error, KafkaStreams#state() will return >> >>>>> State.DISCONNECTED.", which seems to mean that DISCONNECTED should >> be >> >>>>> the >> >>>>> state when the KStreams app is down. >> >>>>> >> >>>>> I wouldn't expect a KIP vote to pass if these basic questions aren't >> >>>>> properly sorted out in the KIP. >> >>>>> >> >>>>> Best, >> >>>>> Michael >> >>>>> >> >>>>> >> >>>>> >> >>>>> On Wed, Apr 17, 2019 at 3:35 AM Richard Yu < >> yohan.richard...@gmail.com> >> >>>>> wrote: >> >>>>> >> >>>>>> Hi all, >> >>>>>> >> >>>>>> Considering that this is a simple KIP, I would probably start the >> >>>>> voting >> >>>>>> tomorrow. >> >>>>>> I think it would be good if we could get this in fast. >> >>>>>> >> >>>>>> On Tue, Apr 16, 2019 at 3:31 PM Richard Yu < >> >>>>> yohan.richard...@gmail.com> >> >>>>>> wrote: >> >>>>>> >> >>>>>>> Oh, I probably misunderstood the difference between DISCONNECTED >> and >> >>>>>> DEAD. >> >>>>>>> I will update the KIP accordingly. >> >>>>>>> Thanks for pointing that out! >> >>>>>>> >> >>>>>>> >> >>>>>>> On Tue, Apr 16, 2019 at 3:13 PM Matthias J. Sax < >> >>>>> matth...@confluent.io> >> >>>>>>> wrote: >> >>>>>>> >> >>>>>>>> Thanks for the initiative. >> >>>>>>>> >> >>>>>>>> In the motivation you mention that you want to use DISCONNECT to >> >>>>>>>> indicate that the application was killed. >> >>>>>>>> >> >>>>>>>> What is the difference to existing state DEAD? >> >>>>>>>> >> >>>>>>>> Also, the backing JIRA seems to have a different motivation to >> add >> >>>>> a >> >>>>>>>> DISCONNECT state. There, the Kafka Streams application itself is >> >>>>>>>> healthy, but it cannot connect to the brokers. It seems >> reasonable >> >>>>> to >> >>>>>>>> add a DISCONNECT for this case though. >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> -Matthias >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> On 4/16/19 9:30 AM, Richard Yu wrote: >> >>>>>>>>> Hi all, >> >>>>>>>>> >> >>>>>>>>> I like to propose a small KIP on adding a new state to >> >>>>>>>> KafkaStreams#state(). >> >>>>>>>>> It is very simple, so this should pass relatively quickly! >> >>>>>>>>> Here is the discussion link: >> >>>>>>>>> >> >>>>>>>> >> >>>>>> >> >>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-457%3A+Add+DISCONNECTED+status+to+Kafka+Streams >> >>>>>>>>> >> >>>>>>>>> Cheers, >> >>>>>>>>> Richard >> >>>>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>> >> >>>>> >> >>>> >> > >> >>
