Skip to content

Bazel: make git_lfs_probe.py try all available endpoints #17172

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

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Aug 7, 2024

It is advisable to exclude b63bd2a from the diff view: that changes only formatting according to my current IDE settings (and the top level black configuration).

This was confirmed to help a user with a specific git remote configuration in ql.

@redsun82 redsun82 marked this pull request as ready for review August 8, 2024 07:34
@redsun82 redsun82 changed the title Bazel: make git_lfs_probe.py try all available endpoints Bazel: make git_lfs_probe.py try all available endpoints Aug 8, 2024
@redsun82 redsun82 requested a review from criemen August 8, 2024 07:36
Copy link
Collaborator

@criemen criemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I wonder if we could somehow persist which endpoint we chose at least for the duration of a single bazel invocation, so that we don't run into timeouts over and over.
But maybe that's
a) overkill to do up-front, and this should be deferred until we observe a need for this in practice
b) quite tricky, as we'd probably need a repository rule to figure this out and store the result (plus, if we don't rerun it every time, that needs cache invalidation too).

@redsun82
Copy link
Contributor Author

redsun82 commented Aug 8, 2024

LGTM! I wonder if we could somehow persist which endpoint we chose at least for the duration of a single bazel invocation, so that we don't run into timeouts over and over. But maybe that's a) overkill to do up-front, and this should be deferred until we observe a need for this in practice b) quite tricky, as we'd probably need a repository rule to figure this out and store the result (plus, if we don't rerun it every time, that needs cache invalidation too).

Indeed. Also, consider that while we might encounter timeouts or delays multiple times in a bazel invocation, we will still encounter them just once per file: after a download succeeds, it will be in the repository cache and be fetched instantaneously from then on. And CI does not have any such problem (the first and only endpoint is good). So I don't think it's worth to complicate it further (considering it already covers a bit of a corner case).

@redsun82 redsun82 merged commit 875d1d3 into main Aug 8, 2024
17 checks passed
@redsun82 redsun82 deleted the redsun82/bazel-lfs branch August 8, 2024 09:06
@criemen
Copy link
Collaborator

criemen commented Aug 8, 2024

Ah that's a good point indeed - where we download a lot, we only have one endpoint, so no confusion possible, and locally we ought to not download a lot at all - understood!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants