-
Notifications
You must be signed in to change notification settings - Fork 320
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
feat: implement backoff for snowpipe streaming authorization errors #5399
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5399 +/- ##
==========================================
- Coverage 74.82% 74.80% -0.03%
==========================================
Files 438 438
Lines 61331 61404 +73
==========================================
+ Hits 45893 45935 +42
- Misses 12895 12931 +36
+ Partials 2543 2538 -5 ☔ View full report in Codecov by Sentry. |
9c22207
to
476cdb4
Compare
476cdb4
to
5644298
Compare
5644298
to
141b31f
Compare
router/batchrouter/asyncdestinationmanager/snowpipestreaming/snowpipestreaming_test.go
Outdated
Show resolved
Hide resolved
router/batchrouter/asyncdestinationmanager/snowpipestreaming/types.go
Outdated
Show resolved
Hide resolved
router/batchrouter/asyncdestinationmanager/snowpipestreaming/snowpipestreaming.go
Outdated
Show resolved
Hide resolved
router/batchrouter/asyncdestinationmanager/snowpipestreaming/snowpipestreaming.go
Outdated
Show resolved
Hide resolved
router/batchrouter/asyncdestinationmanager/snowpipestreaming/snowpipestreaming.go
Outdated
Show resolved
Hide resolved
aa4f042
to
9779ca7
Compare
9779ca7
to
e268f73
Compare
backoff.WithMaxElapsedTime(0), | ||
backoff.WithMaxInterval(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Should we set MaxInterval as say 1 hr? This would mean once issue is resolved around snowflake permissions it will resume working by next 1 hour? Also from correctness perspective, overflow in backoff package is handled as maxInterval.
- Are we using
cenkalti/backoff/v4
elsewhere as well. I see in v5 pattern of initializing has been changed.
CC: @achettyiitr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using cenkalti/backoff/v4 elsewhere as well. I see in v5 pattern of initializing has been changed.
In rudder-server, we are still using v4. We haven't upgraded to v5 yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Made MaxInterval as 1hr
if sfConnectionErr.code == errAuthz { | ||
m.authzBackoff.set() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this mean we end up invoking .NextBackoff as many times as size of uploadInfos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code to set backoff only once
Description
This PR introduces a backoff mechanism when creating channels in the Snowpipe streaming destination. If an authorization error is encountered at the schema, table, or column level, the backoff ensures that we avoid repeatedly attempting to connect to Snowflake unnecessarily.
authzBackoff
in manager to store backoff related statemanagerCreator
as a field in the manager for making the code testable.Security