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.

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

Catch ZstdError in jlap/repodata.json.zst handling #13559

Merged
merged 5 commits into from Feb 9, 2024

Conversation

dholth
Copy link
Contributor

@dholth dholth commented Feb 2, 2024

Description

Fix #13558

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@dholth dholth requested a review from a team as a code owner February 2, 2024 13:31
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Feb 2, 2024
Copy link

codspeed-hq bot commented Feb 2, 2024

CodSpeed Performance Report

Merging #13559 will create unknown performance changes

Comparing 13558-repodata-zst-fallback (5518720) with main (b60f12b)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.

except (JlapSkipZst, HTTPError) as e:
except (JlapSkipZst, HTTPError, zstandard.ZstdError) as e:
if isinstance(e, zstandard.ZstdError):
# will this include the token
Copy link
Member

Choose a reason for hiding this comment

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

I think that's reasonable, so maybe sanitize it? Or leave out the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved.

jezdez
jezdez previously approved these changes Feb 2, 2024
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

Not sure if we actually need the debugging statement, but I concur it's good to catch zstandard decompression exceptions

@dholth
Copy link
Contributor Author

dholth commented Feb 2, 2024

In general this entire code path could be more clear about whether the user's web server is configured correctly, and which variant they wind up getting. If we don't say something then the user would not know that they have a "404 page that returns a 200 code" for repodata.json.zst

For the unchanged code possibly more HTTP errors should result in "use repodata.json and don't check repodata.json.zst for another week"

@jezdez
Copy link
Member

jezdez commented Feb 2, 2024

Then lets make this and the other possible cases result in more specific (and actionable) logging statement? Info level?

@dholth
Copy link
Contributor Author

dholth commented Feb 5, 2024

Improved log message with sanitized URL, and a bonus sanitized URL in a similar log message.

@dholth dholth enabled auto-merge (squash) February 5, 2024 20:47
@dholth dholth force-pushed the 13558-repodata-zst-fallback branch 2 times, most recently from e2bbe6b to 9c73315 Compare February 6, 2024 20:14
@dholth dholth force-pushed the 13558-repodata-zst-fallback branch from 9c73315 to f24aac3 Compare February 6, 2024 20:42
@dholth dholth merged commit 234f74d into main Feb 9, 2024
75 checks passed
@dholth dholth deleted the 13558-repodata-zst-fallback branch February 9, 2024 20:12
kenodegard pushed a commit to kenodegard/conda that referenced this pull request Feb 12, 2024
* catch ZstdError in jlap/repodata.json.zst handler
kenodegard added a commit that referenced this pull request Feb 13, 2024
* Catch ZstdError in jlap/repodata.json.zst handling (#13559)
* change retry logic for 400 <= status < 500 (#13576)

---------

Co-authored-by: Daniel Holth <dholth@anaconda.com>
@jezdez jezdez mentioned this pull request Mar 11, 2024
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fallback to repodata.json if repodata.json.zst raises a zstandard exception
4 participants