Repository navigation
fix: start WritesRetry max_retry_time at a request's first failure - #725
Open
RishavTiwari25 wants to merge 2 commits into
Open
RishavTiwari25 wants to merge 2 commits into
RishavTiwari25 wants to merge 2 commits into
Conversation
WritesRetry computed its retry deadline in __init__ and copied it to every
object returned by increment(). urllib3 starts each request from the
strategy passed as retries to the pool, so an instance given to
InfluxDBClient(retries=...), as the import examples do, shared one
deadline across all requests: once max_retry_time (default 180 s) had
passed since the client was created, every failed write raised
MaxRetryError("max_retry_time exceeded") without a single retry.
The deadline is now set on a request's first failure and carried by the
retry objects of that request only. The check uses <= so that
max_retry_time=0 disables retries even when datetime.now() returns the
same value twice (common on Windows).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #725 +/- ##
==========================================
+ Coverage 90.49% 90.50% +0.01%
==========================================
Files 40 40
Lines 3568 3572 +4
==========================================
+ Hits 3229 3233 +4
Misses 339 339 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
WritesRetrystops retrying oncemax_retry_timehas passed since the strategy object was created, not since the request started failing.__init__setself.retry_timeout = datetime.now() + timedelta(seconds=max_retry_time)andnew()copied it into every object thatincrement()returns. urllib3 starts every request from the strategy given to the pool, so aWritesRetrypassed toInfluxDBClient(retries=...), which is whatexamples/import_data_set_sync_batching.pyandexamples/write_batching_by_bytes_count.pydo, has one deadline shared by all requests. With the defaultmax_retry_time=180, any write that fails more than 3 minutes after the client was created raisesMaxRetryError("max_retry_time exceeded")on its first failure, with no retry. For a long import or a long-running service, that means retries are effectively disabled after startup.(The batching
WriteApiis not affected because it builds a newWritesRetryper batch withWriteOptions.to_retry_strategy().)Fix:
retry_timeoutstarts asNone. On a request's first failure (increment()on an object without a deadline) it is set tonow + max_retry_timeand given to that request's retry objects only. The base strategy is never mutated, so the next request gets its own deadline. The comparison is now<=, somax_retry_time=0reliably disables retries: with<, twodatetime.now()calls in a row can return the same value (often on Windows) and the first retry went through.Tests in
tests/test_WritesRetry.py:test_max_retry_time_starts_at_first_failure: a strategy created 1.5 s before its first failure, withmax_retry_time=1, still retries;test_max_retry_time_is_per_request: a request past its deadline raises, and a new request from the same strategy gets a freshmax_retry_time;test_max_retry_time_zero_disables_retry.All three fail on
master(the last one only on Windows timer resolution); the existingtest_backoff_max_timeand the other retry tests pass unchanged.Checklist
pytest testscompletes successfully: full integration suite not run (needs a live InfluxDB). Focused:pytest tests/test_WritesRetry.py tests/test_WriteOptions.py tests/test_WriteApiBatching.py(47 passed;test_batching_data_frameneeds pandas andtest_WriteApiPickleneeds a running InfluxDB, and both fail the same way onmasterlocally)flake8andpydocstyleare clean on the changed file.