-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add throttling counter in gcsio and refactor retrying #32428
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
- Remove extra retries for copy, delete, _gcs_object. - Remove the use of client.batch() as the function has no built-in retry.
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
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.
Thanks, the uploader / downloader part look good to me. Had an efficiency concern on removing batch operations. Should be easy to resolve if you agree.
Additionally, the variable name for the new retry object is changed. Add a new pipeline option to enable the use of blob generation to mitigate race conditions (at the expense of more http requests)
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.
Thanks, just had a minor comment
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #32428 +/- ##
============================================
+ Coverage 58.86% 58.88% +0.02%
Complexity 3072 3072
============================================
Files 1128 1129 +1
Lines 173921 173968 +47
Branches 3328 3328
============================================
+ Hits 102374 102442 +68
+ Misses 68204 68183 -21
Partials 3343 3343
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The failed YAML Xlang Direct test is unrelated to the code change here. |
* Add retry instance that records throttling metric. * Use retry with throttling counters by default. Add pipeline option. * Fix lint * Fix broken tests. * Retrieve a more accurate throttling time from the caller frame. * Apply yapf and linter * Refactoring copy and delete - Remove extra retries for copy, delete, _gcs_object. - Remove the use of client.batch() as the function has no built-in retry. * Fix a typo and apply yapf * Use counter instead of counters in pipeline option. Additionally, the variable name for the new retry object is changed. Add a new pipeline option to enable the use of blob generation to mitigate race conditions (at the expense of more http requests) * Parameterize existing tests for the new pipeline options. * Apply yapf * Fix a typo. * Revert the change of copy_batch and delete_batch and add warning in their docstring. * Fix lint * Minor change according to code review. * Restore the previous tox.ini that got accidentally changed.
Implement an on_error callback where throttling counter is incremented when GCS throttling happens.
Both copy_batch and delete_batch are reimplemented without client.batch() so retry will happen if a retriable exception is caught.--no_gcsio_throttling_counter
and--enable-gcsio-blob-generation
.This is a second attempt for this feature.
The previous PR is at #31584.