-
Notifications
You must be signed in to change notification settings - Fork 46
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
Implement retry logic for List() Operations #33
Conversation
List() calls will retry until successful or the context is done. PageToken() is used to avoid re-trying successful pages.
@bowei Can you review this since you're the most familiar with this code? Also if you have any suggestions on unit testing that would be appreciated; we don't actually run any of the gen functions in the unit tests. |
@spencerhance, @bowei : Thank you for looking into this! If you think this is the way to go, I can help testing this patch in our CI for a couple of weeks. |
[verifying that prow webhooks work, please excuse this] |
Hi @spencerhance, @bowei. Do you have any thoughts/concerns on this PR? |
if nextPageToken != "" { | ||
call.PageToken(nextPageToken) | ||
} | ||
if err := call.Pages(ctx, f); err != nil { |
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 there different kinds of errors we want to handle differently?
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.
I don't think so, are there any specific response codes you think we should handle? We don't handle response codes individually in the other functions AFAIK
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.
@bowei @spencerhance please let me know if you need any help with this! We're seeing some flakes in our CI that I think would be solved with this patch.
@bowei @spencerhance do you need any help testing this? |
@bowei Is there anything else I need to do to close this out? |
Can we address the question about the kinds of errors we would or wouldn't want to retry? Also, I get concerned that current calls to List() that return with an error when called with a no deadline Context will now just hang. |
Spencer can you update or abandon this PR? |
Closing for now since it's somewhat of a high risk / low reward PR - callers can always just wrap List() in a retry loop. |
List() calls will retry until successful or the context is done. PageToken()
is used to avoid re-trying successful pages.
This should resolve #29
Up for review but hold off on merging since I'm trying to figure out a better way to test this.