Skip to content

Use standard retry logic in _download_ranged - #938

Open
niukanen1 wants to merge 4 commits into
Open-EO:masterfrom
niukanen1:issue934-standard-retry
Open

Use standard retry logic in _download_ranged#938
niukanen1 wants to merge 4 commits into
Open-EO:masterfrom
niukanen1:issue934-standard-retry

Conversation

@niukanen1

@niukanen1 niukanen1 commented Sep 17, 2026

Copy link
Copy Markdown

Fixes #934

Removes the hand-rolled per-range retry loop from _download_ranged. Transient failures are already retried by the urllib3 Retry configuration mounted on the connection's session, so the DIY loop only added divergent behavior: different retry counts, a different status-code set (408/500/501 retried here but not by the session), and OpenEoApiPlainError where the session raises RetryError

The unused MAX_DOWNLOAD_RETRIES_PER_RANGE / RETRIABLE_DOWNLOAD_STATUSCODES constants are gone

Tested with pytest tests/rest/test_download_ranged_retry.py tests/rest/test_job.py: 133 passed — including a new httpretty-based test proving a 503 on a range is retried by the session and the download completes

@niukanen1
Use standard retry logic in _download_ranged
1e5531d 
Remove the hand-rolled per-range retry loop (MAX_DOWNLOAD_RETRIES_PER_RANGE
+ RETRIABLE_DOWNLOAD_STATUSCODES) from _download_ranged. Transient
failures are already retried by the urllib3 Retry configuration mounted
on the connection's session (session_with_retries), so the DIY loop only
added divergent behavior: different retry counts, different status codes
(408/500/501 were retried here but not by the session), and
OpenEoApiPlainError raised where the session raises RetryError.

Fixes Open-EO#934
soxofaan
soxofaan reviewed Sep 21, 2026
View reviewed changes

@soxofaan soxofaan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hi, thanks for contributing

some notes on you PR

Comment thread tests/rest/test_download_ranged_retry.py Outdated
Comment thread CHANGELOG.md Outdated
Comment thread tests/rest/test_download_ranged_retry.py Outdated
Comment thread tests/rest/test_download_ranged_retry.py Outdated
@niukanen1
Address review: move retry test to test_job.py, use download_url
d6a57c6 
- the retry regression test lives in test_job.py next to the other
  ranged download tests, and drives the public download_url method
- the test content uses distinct chunks so each 206 range is unique
- changelog entry moved to the bottom of the Changed section
@niukanen1

niukanen1 commented Sep 21, 2026

Copy link
Copy Markdown
Author

Thanks for the review — addressed all four points in d6a57c6:

  • the test now lives in test_job.py, next to the other ranged download tests
  • it drives the public download_url method instead of _download_ranged
  • the test content is built so each range chunk is unique (distinct byte pattern per 500-byte block, asserted in the test)
  • the changelog entry moved to the bottom of the Changed section

soxofaan
soxofaan reviewed Sep 24, 2026
View reviewed changes

@soxofaan soxofaan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also think the changelog entry needs alignment with latest master

Comment thread tests/rest/test_job.py Outdated
httpretty.register_uri(
httpretty.GET,
uri=API_URL + "/.well-known/openeo",
body=json.dumps({"api_version": "1.0.0", "endpoints": [{"path": "/credentials/basic", "methods": ["GET"]}]}),

@soxofaan soxofaan Sep 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not the correct body to return on /.well-known/openeo
I'd try with not mocking this request, I think that should still work

Comment thread tests/rest/test_job.py Outdated
httpretty.register_uri(
httpretty.HEAD,
uri=API_URL + "/dl/ranged.bin",
body=content.decode("latin-1"),

@soxofaan soxofaan Sep 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a HEAD request doesn't need a body, or am I misunderstanding?

Comment thread tests/rest/test_job.py Outdated
"""#934: transient failures during ranged download are retried by the
standard urllib3 Retry of the connection's session, not a custom loop."""
content = b"R4ng3d-D4t4" * 32 # 352 bytes -> one full and one partial 256-byte range
assert len(set(content[i : i + 256] for i in range(0, len(content), 256))) == 2

@soxofaan soxofaan Sep 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still find this content juggling a bit convoluted

why not return just "A" * 200 on the first GET, "B" * 100 on the second and check that you got "A" * 200 + "B" * 100 in the end?

Comment thread openeo/rest/_connection.py Outdated
) -> None:
# Retries on transient failures (429/502/503/504) are handled by the
# urllib3 Retry mounted on this connection's session (see
# session_with_retries), so no per-range retry loop is needed here.

@soxofaan soxofaan Sep 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find this comment a bit misleading as it is (at this point) not guaranteed what kind of session or mounts are active

@niukanen1
Merge master to align changelog and test fixtures
6aa33bd
@niukanen1
Address review: simplify ranged-retry test mocks and fix comment
690e3d3 
- drop the /.well-known/openeo mock: version discovery fails leniently
- use plain 'A'*200 + 'B'*100 content instead of the chunk-uniqueness juggling
- document why the HEAD registration still needs a body (httpretty constraint)
- reword the _download_ranged comment: retry behavior depends on the
  session setup, which is not guaranteed at this point
@niukanen1

niukanen1 commented Sep 25, 2026

Copy link
Copy Markdown
Author

Addressed all five points in 690e3d3:

  • the /.well-known/openeo mock is gone — the discovery request now fails leniently and falls back to the given url, which is closer to how a real backend interaction starts
  • the HEAD registration no longer pretends to return a real body: it is just a placeholder to satisfy httpretty's Content-Length validation, with a comment explaining why
  • the content juggling is replaced with plain "A" * 200 + "B" * 100 and a direct A*200 + B*100 assertion
  • the misleading comment in _download_ranged is reworded: it now says the session is expected to have urllib3 Retry mounted (as done by default in openeo.connect) instead of asserting it as a guarantee

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soxofaan soxofaan soxofaan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use standard retry logic in _download_ranged

2 participants

@niukanen1 @soxofaan