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: do not continue processing records on permission errors in schema registry #10103

Merged
merged 3 commits into from Nov 6, 2023

Conversation

lucasbru
Copy link
Member

@lucasbru lucasbru commented Nov 3, 2023

Description

KSQL currently treats an exception due to missing permission on schema registry during deserialization as any other deserialization exception, and the default action for deserialization exception is log and continue.

The permission issue should stop KSQL from processing.

With this change:

  • We scan the exception in the LogMetricAndContinueExceptionHandler for a RestClientException (can have multiple levels of wrapping) to get the error code and check if we have to deal with an authorization error.
  • Return FAIL from the deserializationExceptionHandler for such exceptions.
  • The exception will fall through to StreamsUncaughtExceptionHandler. There, we make sure that the exception is classified as a USER error so that on-call is not alerted. The uncaught exception handler will restart the thread and retry processing records.

The StreamThreads for that query will end up in a loop that will restart the StreamThread and retry processing the next record. So we effectively get that the query is blocked until the permission error is resolved.

Testing done

Tested the change in a local setup connected to ccloud schema registry.

Unit tests included.

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 #")
  • Do these changes have compatibility implications for rollback? If so, ensure that the ksql command version is bumped.

@lucasbru lucasbru requested a review from a team as a code owner November 3, 2023 10:00
Copy link

cla-assistant bot commented Nov 3, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

1 similar comment
Copy link

cla-assistant bot commented Nov 3, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lucasbru lucasbru requested a review from spena November 3, 2023 10:01
spena
spena previously approved these changes Nov 3, 2023
Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

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

I left a couple of comments of using the Throwables class instead of using recursion ourselves. The logic is good, so +1 for this PR. But If you can use Throwables, that will make the code simpler I think.

@lucasbru
Copy link
Member Author

lucasbru commented Nov 3, 2023

@spena thanks for the review. I updated the code, seems like you'll need to reapprove it

@suhas-satish suhas-satish requested a review from a team November 3, 2023 15:45
Copy link
Member

@suhas-satish suhas-satish left a comment

Choose a reason for hiding this comment

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

Approved based on Sergio's recent comment , I see throwable has been used. Thanks @lucasbru

@lucasbru lucasbru merged commit bca033d into confluentinc:master Nov 6, 2023
3 of 4 checks passed
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.

None yet

3 participants