Thanks for the updates Walker. They all lgtm. On Tue, Sep 29, 2020 at 8:33 AM Walker Carlson <wcarl...@confluent.io> wrote:
> 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 > > > > > > > > -- -- Guozhang
