Thank you for the feedback Guozhang and Bruno. See the responses below. I have updated the kip accordingly
Thanks, Walker On Tue, Sep 29, 2020 at 1:59 AM Bruno Cadonna <br...@confluent.io> wrote: > Hi Walker, > > Thanks for updating the KIP! > > 1. I would add response REPLACE_STREAM_THREAD to the > StreamsUncaughtExceptionHandlerResponse enum to start a new stream > thread that replaces the failed one. I suspect you did not add it > because it depends on KIP-663. A dependency to another unfinished KIP > should not stop you from adding this response. > Yes I was unsure about adding this as your kip is still under discussion. We can add it to 663 if you would prefer then there will not be any dependency on it. We can always add it separately or we can do a partial implementation. Whichever you think is best > > 2. Why does the Kafka Streams client transit to NOT_RUNNING when it is > shutdown due to SHUTDOWN_KAFKA_STREAMS_CLIENT and > SHUTDOWN_KAFKA_STREAMS_APPLICATION? I would rather expect that it > transits to ERROR, since we are exclusively talking about error cases > now. I would also not emulate the current behavior of close(), since > close() is not intended to be used in the error case due to deadlocks > you could run into. > We can change it to state Error. > > 3. Since the motivation of the KIP changed quite a lot, I think you > should remove KAFKA-4748 from the motivation or make it clear that this > KIP does only cover the shutdown of the Kafka Streams application in the > error case. > > This is a fair point. I will remove it. > 4. I would just overload method setUncaughtExceptionHandler() and not > introduce a method with a new name. > Alright. It is the same reason I hadn't deprecated it but I think we can just go a head a do it. > > 5. I agree with Guozhang that we should deprecate the overload with the > Java-specific handler. I am sure you wanted to deprecate the method and > just forgot about it. > I actually had but removed it because I felt that we are not replacing it completely, but we might as well depreate it. > > 6. I agree with Guozhang that the RocksDB metrics recording thread > should also be shut down. To be fair, when Walker asked me about it, I > thought it is not strictly necessary to shut it down, but thinking about > it again, it also does not make a lot of sense to keep it running, > because the RocksDB metrics would have all be removed at that point. > We can change that > > 7. I think we should provide a default implementation of the handler. > However, the default implementation should just return > SHUTDOWN_STREAM_THREAD which corresponds to the current behavior. If we > want to provide a more elaborated default handler, I would propose to > discuss that on a separate KIP to not block this KIP on that discussion. > This is what I am currently doing. Before it is set it defaults to a lambda to just SHUTDOWN_STREAM_THREAD and if they reset if by passing null it will return to the default. I agree that a default that changes the behavior of the default might want to wait. > > Best, > Bruno > > On 29.09.20 05:35, Guozhang Wang wrote: > > Hello Walker, > > > > Thanks for the updated KIP proposal. A few more comments below: > > > > 1. "The RocksDB metrics recording thread is not shutdown." Why it should > > not be shut down in either client or application shutdown cases? > see above, it's been fixed > > > > 2. Should we deprecate the existing overloaded function with the java > > UncaughtExceptionHandler? > If we are going to depreciate it then yes. I have updated it > > > > 3. Should we consider providing a default implementation of this handler > > interface which is automatically set if not overridden by users, e.g. one > > that would choose to SHUTDOWN_KAFKA_STREAMS_APPLICATION upon > > MissingSourceTopicException in KIP-662. > We could but I don't think it's strictly necessary though. > > > > > > Guozhang > > > >
