feat: apply exponential backoff when mergeable status is unknown #44
Conversation
Pull Request Test Coverage Report for Build 18318656495Details
💛 - Coveralls |
zhiyelee
left a comment
zhiyelee
left a comment
There was a problem hiding this comment.
LGTM overall. left a comment
| // https://docs.github.com/en/rest/guides/getting-started-with-the-git-database-api#checking-mergeability-of-pull-requests | ||
| // Github recommends to use poll to get a non null/unknown value, we use a compromised version here because of the api rate limit | ||
| await wait(3000); | ||
| await wait(3000 * Math.pow(2, attempt)); |
There was a problem hiding this comment.
Is the duration too long?
There was a problem hiding this comment.
Is the duration too long for what? Here I took the existing three seconds that was in place and applied exponential backoff. Since the maximum value of attempt will be 4 this will lead to a value of 48 seconds for the worst case being passed as the argument to the wait function. I think the default timeout for a github workflow is 6 hours. There would have to be thousands of open PRs that all reached the 48 second timeout for this value to be an issue. But in that case we would have waited a long time for every open pull request and for whatever reason github continued to have a value of unknown as the mergeable state. In this case github is most likely having some kinds of outage.
There was a problem hiding this comment.
makes sense. Have you tested this and confirmed that this wont trigger rate limit?
There was a problem hiding this comment.
No but according to github's documentation we should be at least a thousand requests per hour. I also don't think it is feasible for someone to try to test this. They could see a few passing requests and not hit the rate limit but that doesn't indicate that this change isn't going to ever trigger the rate limit.
Whether this change leads to more rate limit triggers will depend on the number of open PRs that are waiting to merge to master. In cases where the merge status is not known we will end up making 4 more requests. If every PR were to run into this case we will then make those 4 requests again and have more requests per hour.
If you are concerned about reaching the wait limit we could instead change the default 3 seconds to a larger value. This would allow github more time to determine the status and decrease the potential of a rate limit error as we will be making less requests per hour. If we changed it to say 30 seconds we would give github more time to determine the status and have less requests per hour decreasing risk of triggering rate limit errors.
In cases where the mergeable status is
unknownwe wait 3 seconds and make another request to determine the pull request status. If 3 seconds isn't enough time it will be the case that we still get a mergeable status ofunknown. This change adds exponential backoff and tries a few more times allowing for more time for the status mergeable status to update.