Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Limit wait for slow mirrors #36565

Merged
merged 1 commit into from
Nov 5, 2023
Merged

Limit wait for slow mirrors #36565

merged 1 commit into from
Nov 5, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 28, 2023 •
edited
Loading

Using a timeout of 1 second unconditionally, stopping when 5 good mirrors (300ms ping) are found

Fixes #34411

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe
build/sage_bootstrap/download/mirror_list.py: Use socket timeout of 1…
b6c05e0 
…s unconditionally, stop when 5 good mirrors are found
@mkoeppe mkoeppe requested a review from dimpase October 28, 2023 22:12
@mkoeppe mkoeppe self-assigned this Oct 28, 2023
@mkoeppe mkoeppe requested a review from kwankyu October 28, 2023 22:24
@github-actions
Copy link

Documentation preview for this PR (built with commit b6c05e0; changes) is ready! 🎉

@mkoeppe mkoeppe added this to the sage-10.2 milestone Oct 29, 2023
kwankyu
kwankyu reviewed Oct 29, 2023
View reviewed changes
build/sage_bootstrap/download/mirror_list.py
if len(timed_mirrors) >= 5 and timed_mirrors[4][0] < 0.3:
# We don't need more than 5 decent mirrors
break

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where that number 5 comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just feeling confident that when 5 mirrors say they don't have the file, it's time to contact the upstream url.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I don't know how things here work, I guessed that it is related with the maximum concurrent number of downloads of packages and wondered how that number is determined to be 5...

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Now I learned a little how things work here. A "decent" mirror is a site that allows fast connection and hence the number of them has nothing to do with "concurrency".

Sorry for noise.

kwankyu
kwankyu approved these changes Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 29, 2023 •
edited
Loading

Do you expect that this may solve the timeout problem observed on ubuntu-mantic? Or not related?

@dimpase
Copy link
Member

dimpase commented Oct 29, 2023

1 sec timeout might be too optimistic for slow networks.

We need an option to override this if needed

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 29, 2023

Do you expect that this may solve the timeout problem observed on ubuntu-mantic?

Yes, it might

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 29, 2023

1 sec timeout might be too optimistic for slow networks.

This PR does not adjust the time.

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 1, 2023
8f4a406 
sagemathgh-36565: Limit wait for slow mirrors
    
Using a timeout of 1 second unconditionally, stopping when 5 good
mirrors (300ms ping) are found

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
Fixes sagemath#34411
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36565
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 2, 2023
964b9f9 
sagemathgh-36565: Limit wait for slow mirrors
    
Using a timeout of 1 second unconditionally, stopping when 5 good
mirrors (300ms ping) are found

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
Fixes sagemath#34411
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36565
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@dimpase
Copy link
Member

dimpase commented Nov 2, 2023

I'm getting problems like

[wheel-0.41.2] Attempting to download package wheel-0.41.2.tar.gz from mirrors
[wheel-0.41.2] https://github.com/sagemath/sage/releases/download/10.2/wheel-0.41.2.tar.gz
[wheel-0.41.2] [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
[wheel-0.41.2] ERROR [transfer|run:135]: [Errno socket error] [Errno 404] Not Found: '//github.com/sagemath/sage/releases/download/10.2/wheel-0.41.2.tar.gz'
[wheel-0.41.2] https://github.com/sagemath/sage/releases/download/10.1/wheel-0.41.2.tar.gz
[wheel-0.41.2] [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx]
[wheel-0.41.2] ERROR [transfer|run:135]: [Errno socket error] [Errno 404] Not Found: '//github.com/sagemath/sage/releases/download/10.1/wheel-0.41.2.tar.gz'

with this - something is wrong...

@dimpase
Copy link
Member

dimpase commented Nov 2, 2023

fixed by removing upstream/mirror_list. Duh...

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 2, 2023

You get the 404 from GitHub releases because we haven't deposited upstream tarballs there yet

@vbraun vbraun merged commit e6c9662 into sagemath:develop Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dimpase dimpase dimpase approved these changes

@kwankyu kwankyu kwankyu approved these changes

Assignees

@mkoeppe mkoeppe

Labels
c: build p: blocker / 1
Projects
None yet
Milestone
sage-10.2
Development

Successfully merging this pull request may close these issues.

sage-download-file: Limit number of mirrors contacted
4 participants
@mkoeppe @kwankyu @dimpase @vbraun