Hello Guozhang and Bruno, Thanks for the feedback.
I will respond in two parts but I would like to clarify that I am not tied down to any of these names, but since we are still deciding if we want to have an exception or not I would rather not get tripped up on choosing a name just yet. Guozhang: 1) you are right about the INCOMPLETE_SOURCE_TOPIC_METADATA error. I am not planning on changing the behavior of handling source topic deletion. That is being down in kip-662 by Bruno. He is enabling the user to create their own handler and shutdownApplication is giving them the option to shutdown. 2) It seems that we will remove the Exception entirely so this won't matter (below) 3) It should wait to receive the shutdown request in the rebalance it triggers. That might be a better name. I am torn between using "application" or "all Instances" in a couple places. I think we should pick one and be consistent but I am unsure which is more descriptive. Bruno: I agree that in principle Exceptions should be used in exception cases. And I have added a method in KafkaStreams to handle cases where an Exception would not be appropriate. I guess you think that users should never throw a Streams Exception then they could always throw and catch their own exception and call shutdown Application from there. This would allow them to exit a processor if they wanted to shutdown from there. I will update the Kip to remove the exception. I would like to add that in the case of trying to shutdown from the uncaught exception handler that we need at least one StreamThread to be alive. So having our own handler instead of using the default one after the thread has died would let us always close the application. Walker On Wed, Sep 16, 2020 at 5:02 AM Bruno Cadonna <br...@confluent.io> wrote: > Hi Walker, > > Thank you for the KIP! > > I like the motivation of the KIP and the method to request a shutdown of > all Kafka Streams clients of Kafka Streams application. I think we > really need such functionality to react on errors. However, I am not > convinced that throwing an exception to shutdown all clients is a good > idea. > > An exception signals an exceptional situation to which we can react in > multiple ways depending on the context. The exception that you propose > seems rather a well defined user command than a exceptional situation to > me. IMO, we should not use exceptions to control program flow because it > mixes cause and effect. Hence, I would propose an invariant for public > exceptions in Kafka Streams. The public exceptions in Kafka Streams > should be caught by users, not thrown. But maybe I am missing the big > advantage of using an exception here. > > I echo Guozhang's third point about clarifying the behavior of the > method and the naming. > > Best, > Bruno > > On 16.09.20 06:28, Guozhang Wang wrote: > > Hello Walker, > > > > Thanks for proposing the KIP! I have a couple more comments: > > > > 1. ShutdownRequestedException: my understanding is that this exception is > > only used if the application-shutdown was initiated by by the user > > triggered "shutdownApplication()", otherwise e.g. if it is due to source > > topic not found and Streams library decides to close the whole > application > > automatically, we would still throw the original exception > > a.k.a. MissingSourceTopicException to the uncaught exception handling. Is > > that the case? Also for this exception, which package are you proposing > to > > add to? > > > > 2. ShutdownRequestedException: for its constructor, I'm wondering what > > Throwable "root cause" could it ever be? Since I'm guessing here that we > > would just use a single error code in the protocol still to tell other > > instances to shutdown, and that error code would not allow us to encode > any > > more information like root causes at all, it seems that parameter would > > always be null. > > > > 3. shutdownApplication: again I'd like to clarify, would this function > > block on the local instance to complete shutting down all its threads > like > > `close()` as well, or would it just to initiate the shutdown and not wait > > for local threads at all? Also a nit suggestion regarding the name, if it > > is only for initiating the shutdown, maybe naming as "initiateCloseAll" > > would be more specific? > > > > > > Guozhang > > > > > > > > > > On Mon, Sep 14, 2020 at 10:13 AM Walker Carlson <wcarl...@confluent.io> > > wrote: > > > >> Hello Matthias and Sophie, > >> > >> You both make good points. I will respond to the separately below. > >> > >> > >> Matthias: > >> That is a fair point. KIP-662 > >> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-662%3A+Throw+Exception+when+Source+Topics+of+a+Streams+App+are+Deleted > >>> , > >> which > >> is accepted, will make it so Source topic deletion will make it to the > >> uncaught exception handler. Shutdown can be initiated from there. > However > >> this would mean that the stream thread is already dead. So I would have > to > >> rethink the exception for this use case, perhaps it would be needed in > the > >> KakfaStreams object. But this still leaves the case where there is only > one > >> stream thread. I will think about it. > >> > >> Maybe the source topics are a bad example as it makes this kip > dependent on > >> Kip-662 getting implemented in a certain way. However this is not the > only > >> reason this could be useful here > >> <https://issues.apache.org/jira/browse/KAFKA-4748> is a jira ticket > asking > >> for the same functionality. I have added a few other use cases to the > kip. > >> Although I will still be rethinking where I want to add this > functionality > >> and whether it should be an exception or not. > >> > >> Sophie: > >> I agree that shutting down an instance could also be useful. There was > some > >> discussion about this on KIP-663. It seems that we came to the > conclusion > >> that close(Duration.ZERO) would be sufficient. link > >> < > >> > https://mail-archives.apache.org/mod_mbox/kafka-dev/202008.mbox/%3c95f95168-2811-e57e-96e2-fb5e71d92...@confluent.io%3e > >>> > >> to > >> thread > >> > >> Also I am not set on the name ShutdownRequested. If we decide to keep > at as > >> an exception your idea is probably a better name. > >> > >> > >> Thanks for the feedback, > >> Walker > >> > >> > >> On Fri, Sep 11, 2020 at 11:08 AM Matthias J. Sax <mj...@apache.org> > wrote: > >> > >>> Thanks for the KIP. > >>> > >>> It seem that the new exception would need to be thrown by user code? > >>> However, in the motivation you mention the scenario of a missing source > >>> topic that a user cannot detect, but KafkaStreams runtime would be > >>> responsible to handle. > >>> > >>> How do both things go together? > >>> > >>> > >>> -Matthias > >>> > >>> On 9/11/20 10:31 AM, Walker Carlson wrote: > >>>> Hello all, > >>>> > >>>> I have created KIP-671 to give the option to shutdown a streams > >>>> application in response to an error. > >>>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-671%3A+Shutdown+Streams+Application+when+appropriate+exception+is+thrown > >>>> > >>>> This is because of the Jira ticket > >>>> <https://issues.apache.org/jira/browse/KAFKA-9331> > >>>> > >>>> Please give it a look and let me know if you have any feedback. > >>>> > >>>> Thanks, > >>>> Walker > >>>> > >>> > >>> > >> > > > > >
