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: apply custom error handler to KsqlStatementException (MINOR) #9442

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Aug 11, 2022

Description

The custom error handler in KsqlResource is currently only applied to raw exceptions and KsqlExceptions, but not KsqlStatementExceptions. This means the error handler misses cases such as when Schema Registry is not enabled, because the DefaultSchemaInjector has logic to wrap the KsqlException in a KsqlStatementException.

We could fix this by not having injectors (and other parts of the code) wrap KsqlExceptions, but it seems better to leave that flexibility and instead apply the error handler to KsqlStatementExceptions from KsqlResource. At least, I see no reason not to.

Testing done

Added unit tests.

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 #")

@vcrfxia vcrfxia marked this pull request as ready for review August 11, 2022 18:43
@vcrfxia vcrfxia requested a review from a team as a code owner August 11, 2022 18:43
Copy link
Member

@pgaref pgaref left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Victoria!

Just for completeness, seems like these are the 2 checks we are missing by not invoking the custom error handler getting default responses instead of the specific ones

@vcrfxia vcrfxia merged commit b2b5584 into confluentinc:master Aug 12, 2022
@vcrfxia vcrfxia deleted the ksql-statement-error-handler branch August 12, 2022 15:30
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.

2 participants