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
