Rudimentary Bazel Download Retry Logic #495
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
celestialorb
force-pushed
the
improvement/download/retry
branch
3 times, most recently
from
fabb809
to
7020954
Compare
celestialorb
force-pushed
the
improvement/download/retry
branch
from
51c786d
to
e24ba47
Compare
bazelisk.py
Outdated
| pass | ||
| if creds is not None: | ||
| auth = base64.b64encode(("%s:%s" % (creds[0], creds[2])).encode("ascii")) | ||
| request.add_header("Authorization", "Basic %s" % auth.decode("utf-8")) |
There was a problem hiding this comment.
Can we construct auth and request once and use them across retries ?
There was a problem hiding this comment.
I don't see why not. I've made the changes but I'm hitting issues testing it locally. Don't think it's related to the changes, seems it's just unable to download a specific Bazel checksum file; specifically https://storage.googleapis.com/bazel-builds/artifacts/ubuntu1404/f3aa4184c0b53864f597ecfb8969938e9a2a8287/bazel.sha256
I'll go ahead and push my changes and then try reverting them locally to see if the old code in this PR works or if this is indeed an issue accessing that resource.
There was a problem hiding this comment.
As a side note I also added configuration of the retry logic via environment variables to this PR. If you'd like me to change those variables or how those settings are sourced just let me know.
There was a problem hiding this comment.
I don't see why not. I've made the changes but I'm hitting issues testing it locally. Don't think it's related to the changes, seems it's just unable to download a specific Bazel checksum file; specifically
https://storage.googleapis.com/bazel-builds/artifacts/ubuntu1404/f3aa4184c0b53864f597ecfb8969938e9a2a8287/bazel.sha256I'll go ahead and push my changes and then try reverting them locally to see if the old code in this PR works or if this is indeed an issue accessing that resource.
Figured it out, didn't realize that for the checksum the outer scope was expecting a 404 HTTPError to be thrown if the checksum doesn't exist for the specific Bazel version.
|
@ maintainers - can someone take a look at this PR ? |
celestialorb
force-pushed
the
improvement/download/retry
branch
2 times, most recently
from
8150c31
to
f8a7f11
Compare
|
@akshaysngupta @ maintainers I went ahead and reexamined this one and saw that one of the tests timed out. I pushed an empty commit to retrigger the job and the test successfully passed after that. Should be ready to be merged in if someone could take a look at it. |
|
Should at least help to mitigate #432. |
|
How do we get owner attention on this PR? |
|
@meisterT this is a priority for some of my users who are running into CI flakiness due to Bazel download failures. Who can look at this? |
|
Let me figure out who can do that. Note that this seems to touch the python version which is lacking behind in features - out of curiosity: why are you not using the golang version? |
|
Sorry for delay, but also curious about @meisterT 's question, are you sure the Python version is what you use? |
|
In the event we don't hear back from the original author after 2y, I think we would only fix this in the Go implementation. And should probably have a plan to remove the Python implementation or at least mark it obsolete |
@meisterT Honestly I'm not sure. We were encountering download failures in our CI with Bazelisk and I probably searched for the given error message which lead me to the Python implementation. What's the difference between the two implementations, and why are there two? Is the Golang implementation newer / preferred? If it helps I checked our version of Also, from a quick glance at the Golang code it seems like there's already some rudimentary retry logic for that implementation. If the Golang implementation is preferred I'll need to talk with the team that maintains this at my organization and ask them why we're using the Python implementation. |
|
@celestialorb Yes, the golang version if preferred and I suspect you are already using it since we don't actually have release for the python version. Please check. @alexeagle I agree there should be only one version. But currently there are at least three? I have no idea how the js and py version are used. /cc @fweikert |
|
The unofficial policy has been to accept community contributions to the Python version, but we (Bazel team) release new features for the Go version only. The JS version seems to be stale and should be removed. There's quite a backlog of PRs in general due to a lack of time. I hope this is something we can work through this month. |
I get exceptions on downloading Bazel via Bazelisk quite often and I was surprised to see no retry logic implemented for the download, so I quickly slapped together something basic. Hopefully this'll get the ball rolling on a proper solution.
My initial thought was to use something like
tenacityto wrap the download function, but didn't see an easy way of adding a Python dependency to Bazelisk.There's also the approach of using the native retry handler with
urllib, but I'm not sure if that'll work with Python2.