SNOW-1623269 Fail sink task on authorization exception from Snowflake #916
Conversation
| boolean authorizationTaskFailureEnabled; | ||
| boolean authorizationErrorReported; |
There was a problem hiding this comment.
Sure, done.
| public void reportPrecommitException(Exception ex) { | ||
| if (authorizationTaskFailureEnabled | ||
| && ex.getMessage().contains(AUTHORIZATION_EXCEPTION_MESSAGE)) { | ||
| authorizationErrorReported = true; | ||
| } | ||
| } | ||
|
|
||
| /** Throw exception if authorization has failed before */ | ||
| public void throwExceptionIfAuthorizationFailed() { | ||
| if (authorizationTaskFailureEnabled && authorizationErrorReported) { | ||
| throw ERROR_1005.getException(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think that in reportPrecommitException() we do not need a feature check. toggling a field is not dangerous and gives us another possibility...
...in throwExceptionIfAuthorizationFailed() if the feature is disabled by default (can we check if it is default vs explicitly disabled?), we could add a warning log — something like:
Authorization problem detected. You may want to configure "enable.task.fail.on.authorization.errors = true" to make the connector automatically pickup rotated credentials
It could help users with discoverability and fixing problem on their own — WDYT?
There was a problem hiding this comment.
I think it might not be the best idea because of the following reasons:
- failing task doesn't mean that it's going to be restarted. This behaviour is specific to Confluent platform.
- authorization failure might happend from different reasons than key rotation
- there would be a lot of this log as they would be produced on every put() call
Overall it's a good idea, but I have doubts about this specific scenario.
There was a problem hiding this comment.
What you have just wrote could be a very fitting warning message then. We could create a latch to print it only once. Food for thought. Anyway — good job :)
Overview
SNOW-1623269
Details of this error can be found in JIRA and comments left in the code. The fix is not very complicated but decided to hide it behind a parameter for safety reasons.
I tested this change end-to-end on sfctest0 cause it's tricky to test it automatically.
Pre-review checklist
enable.task.fail.on.authorization.errors.Yes- Added end to end and Unit Tests.No- Suggest why it is not param protected