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

omit chunks on client HTTP error (like 404, 403,...) #134

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

Alexander-Barth
Copy link
Contributor

@Alexander-Barth Alexander-Barth commented Feb 7, 2024

This PR resolves issue #131

@Alexander-Barth Alexander-Barth changed the title omit chunk on client HTTP error omit chunk on client HTTP error (like 404, 403,...) Feb 7, 2024
@Alexander-Barth Alexander-Barth changed the title omit chunk on client HTTP error (like 404, 403,...) omit chunks on client HTTP error (like 404, 403,...) Feb 7, 2024
@meggart
Copy link
Collaborator

meggart commented Feb 7, 2024

Sorry for missing this issue, luckily you have found out already that currently we only assume a chunk is missing when the server returns 404, which IMO should be the correct code according to e.g. this list https://en.wikipedia.org/wiki/List_of_HTTP_status_codes and so far all providers I have worked with returned this for missing chunks.

All other codes might contain valuable information on why a request did not work, so I would also be hesitant to generally interpret these codes as missing.

I have just pushed a different solution and hope this would work as well for you. In this PR when 403 is returned accessing a chunk you would receive an informative error message and can add the error code to the list of allowed codes that are interpreted as missing chunks. Your example would look like this:

using Revise, Zarr

ds = Zarr.zopen("https://s3.waw3-1.cloudferro.com/mdl-arco-time/arco/MEDSEA_MULTIYEAR_PHY_006_004/med-cmcc-cur-rean-d_202012/timeChunked.zarr")

ds["uo"][:,:,1,1]
#Throws informative error

Zarr.missing_chunk_return_code!(ds.storage,403)
#alternative
Zarr.missing_chunk_return_code!(ds.storage,400:499)

ds["uo"][:,:,1,1]
#No error

@coveralls
Copy link

coveralls commented Feb 7, 2024

Pull Request Test Coverage Report for Build 7826978286

  • 8 of 10 (80.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.3%) to 87.572%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Storage/http.jl 8 10 80.0%
Files with Coverage Reduction New Missed Lines %
src/metadata.jl 1 88.66%
Totals Coverage Status
Change from base Build 6484107136: 0.3%
Covered Lines: 761
Relevant Lines: 869

💛 - Coveralls

@Alexander-Barth
Copy link
Contributor Author

Perfect, yes, your solution is better. For your information, I also informed the data provider (Copernicus Marine, https://marine.copernicus.eu/) about this issue, as the HTTP error 403 (permission denied) is indeed a bit confusing.

@meggart meggart merged commit d932969 into JuliaIO:master Feb 8, 2024
8 of 10 checks passed
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.

None yet

3 participants