Increment retries on tried Chunks#122
Conversation
ReinierMaas
force-pushed
the
reinier/increment-chunk-retries
branch
2 times, most recently
from
bbc10f1 to
995b722
Compare
This roundtrips via the client to prevent network issues from incrementing the retry count.
ReinierMaas
force-pushed
the
reinier/increment-chunk-retries
branch
from
995b722 to
8b6f2c9
Compare
|
The main reason why a reservation expiration exists, is to deal with consumers that are hanging. If they are stuck in an infinite loop with the OS not being overloaded, everything is fine as the Rust async executor in the client can then still make forward progress. But if the consumer machine really has a hard time, we cannot be 100% sure they will respond to a "yo your reservation has expired" message. Note that issues that clearly are network related (graceful and ungraceful 'network closed' as well as 'too many heartbeat failures') do result in the retry count being incremented. I think this is the right choice generally speaking, since it will prevent a submission from ever being infinitely stuck inside opsqueue all-together. So you might be able to simplify this PR further, as I don't think we need to do the round-trip. (I'm perfectly fine with merging this first and simplifying it after if that has your preference though). |
|
I think it makes sense to have different paths for a client that is not responding to health-checks, i.e. it has actual network issues. On the other in the case we can respond to network requests we ensure that the reservation expiration is pushed behind the chunk completion if that already entered the network. This also was easy to generalize to the reserved chunks failure path for which the requester disappeared so that is a nice addition I would say. I would also be fine incrementing the retry count completely server side if that has your preference but the client is responding to health-checks in this case so we can assume it is making forward progress. We have a separate issue to add a cancellation token to the Chunk so that the requester can now that the chunk it is currently processing has expired, we just haven't implemented that and I think it would make sense to do that in the future as well. XRef: #48 |
| if explicit_bin := os.environ.get("OPSQUEUE_BIN", "").strip(): | ||
| return Path(explicit_bin) |
There was a problem hiding this comment.
This was required to reduce the time spent in the setup and teardown fixtures so we can enable test timeouts. The binaries are now computed once in the Just file for all the xdist workers.
|
|
||
| # Timeout (seconds) for Python integration test recipes. | ||
| # Override with OPSQUEUE_PYTEST_TIMEOUT, e.g. OPSQUEUE_PYTEST_TIMEOUT=600 just nix-test-integration | ||
| pytest_timeout_seconds := env_var_or_default("OPSQUEUE_PYTEST_TIMEOUT", "60") |
There was a problem hiding this comment.
Local test timeout is now set at a more reasonable 1 minute instead of the 10 minutes required on CI...
Also the build phase for the binaries is excluded from this timeout to prevent network retries in Nix realise and Cargo fetch from triggering the test timeout.
| cargo clippy --no-deps --fix --allow-dirty --allow-staged -- -Dwarnings | ||
| cargo clippy --no-deps --fix --allow-dirty --allow-staged --no-default-features -- -Dwarnings | ||
| cargo clippy --no-deps --fix --allow-dirty --allow-staged --all-features -- -Dwarnings |
There was a problem hiding this comment.
I was reading through the open PRs and found #109
This showed that we need to extend this even further to make cargo clippy actually fail on warnings, now it only tries to resolve them using --fix and won't exit with a status code for remaining warnings.
| @@ -377,7 +446,7 @@ def test_cancel_submission_not_found( | |||
|
|
|||
|
|
|||
| def test_cancel_in_progress( | |||
| opsqueue: OpsqueueProcess, any_consumer_strategy: StrategyDescription | |||
| opsqueue: OpsqueueProcess, | |||
| ) -> None: | |||
| """Cancelling a submission that is in progress succeeds and returns none. | |||
| Attempting to cancel a submission that is already cancelled should raise a | |||
| @@ -400,7 +469,7 @@ def test_cancel_in_progress( | |||
|
|
|||
|
|
|||
| def test_cancel_already_cancelled( | |||
| opsqueue: OpsqueueProcess, any_consumer_strategy: StrategyDescription | |||
| opsqueue: OpsqueueProcess, | |||
| ) -> None: | |||
There was a problem hiding this comment.
Removing the unused strategies reduced the number of tests that are executed while retaining the same coverage.
| # If after 120sec (accomodating for a toaster-like PC, or an overloaded Semaphore runner) | ||
| # there is no progress, assume a deadlock | ||
| # timeout=120 | ||
| timeout=20 |
There was a problem hiding this comment.
10s did timeout sporadically so I bumped this to 20 seconds. If it turns out not to be enough I would double it again but remain lower than the shorter timeout in the just file (60s/1 minute) so that a timeout timeout doesn't hinder pytest from printing the timeout reason locally.
2 participants
XRef: #48
This roundtrips via the client to prevent network issues from incrementing the retry count.