Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: UncaughtExceptionHandler not being set for Persistent Queries #4087

Conversation

stevenpyzhang
Copy link
Member

@stevenpyzhang stevenpyzhang commented Dec 9, 2019

Description

Fixes #4079
It looks like from the original PR https://github.com/confluentinc/ksql/pull/3425/files to master, there was some refactoring done with how the Streams app is created which inadvertently changed the UncaughtExceptionHandler's behavior. The refactor changed https://github.com/confluentinc/ksql/pull/3425/files#diff-ef0765548f1131d562da8a8ef9edad73R285 was doing. The UncaughtExceptionHandler needs to be set for PersistentQueries (It's set in QueryStreamWriter.java for transient), which is why previously there's an explicit call in the buildPlanForStructuredOutputNode method (this was the pre-refactor method for creating PersistentQueries). The refactor resulted in setUncaughtExceptionHandler() being moved to the buildTransientQuery when it should've been moved to buildQuery
We need to be setting a exception handler for streams threads because they shouldn't pick up the global uncaught exception handler (which terminates the server when hit).

I've moved setting the handler to KafkaStreamsBuilderImpl which is called before creating every Streams App.

This may also need to be backported to 5.4.x as the refactor was before that branch was cut.

Testing done

Tried the scenario outlined in the Github Issue and the server didn't shut down after this change.
Also, manually verified in debug mode that the handlers were set for the streams threads.
Screen Shot 2019-12-09 at 11 26 04 AM

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@stevenpyzhang stevenpyzhang requested a review from a team as a code owner December 9, 2019 19:37
@stevenpyzhang stevenpyzhang requested a review from rodesai December 9, 2019 19:37
@stevenpyzhang stevenpyzhang force-pushed the uncaught-exception-handler-bug branch from 8b741fa to f517cee Compare December 9, 2019 20:05
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stevenpyzhang stevenpyzhang merged commit e193a2a into confluentinc:master Dec 9, 2019
@stevenpyzhang stevenpyzhang deleted the uncaught-exception-handler-bug branch February 6, 2020 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If ksql.server.exception.uncaught.handler.enable is set and a streams thread dies, the server shuts down
2 participants