Repository navigation
Commit d099390
authored
fix(load): submit a managed load as a job and poll, don't hold a request open (#72)
* fix(load): submit a managed load as a job and poll, don't hold a request open
A load's duration scales with the data, so waiting for one inside a single HTTP
request makes it depend on every layer between the client and the engine staying
up for the whole thing -- CDN, gateway, and this client's own socket read
deadline. Any one of them ending the request loses the load: the server marks it
`abandoned` and DISCARDS work it had already done, while the table's write lock is
still held against the retry that follows. The retry then collides with that lock
(409 RESOURCE_LOCKED) and the two can alternate indefinitely without the load ever
completing.
Seen in production on a table whose load runs past five minutes: the same load
acquired, was abandoned at ~4m45s, and re-acquired, three times in a row, while
the caller retried 409s for twenty-five minutes. Small tables on the same
connection succeeded in under two seconds throughout, which is what ruled out the
lock itself being at fault.
The server already offers the fix and recommends it for this exact case -- "run
the load as a background job and return a job ID to poll instead of blocking until
it finishes. Recommended for large uploads, which can take longer than an HTTP
request should stay open." So loads now go out with `async` set.
`async_after_ms` keeps the common case unchanged. The server answers 200 with the
result when the load finishes inside the window and only defers to a job when it
does not, so nothing pays for polling that did not need it -- dlt's bookkeeping
tables settle in under a second and would otherwise gain a submit-then-poll round
trip each.
The polling budget is the load's own, deliberately not the 300s that queries and
results share: reusing that is what put a five-minute ceiling on loads to begin
with. Bounded rather than unbounded, so a wedged job still surfaces as a timeout.
`_poll_job` already existed for the index path, which has had this shape all
along; this reuses it rather than adding a second poller.
Worth noting for `append`: it is the one non-idempotent mode, and the reason a
lost response is dangerous there is that "did it land?" becomes unanswerable. A
job id outlives the request that created it, so that ambiguity is gone -- the
retry rule is unchanged here, but it no longer rests on the response surviving.
Verified against a live deployment: the server accepts the new fields, a fast load
still returns inline, and no polling happens when it does. The deferred path is
covered by unit tests -- reproducing it live needs a load slow enough to exceed
the window, which the reporting workload produces and a synthetic 300k-row load
(0.8s) does not.
* fix(load): a failed status check must not discard a running job
Review found a regression this PR introduced, reachable from the opposite
direction to the one it fixes.
`_poll_job` aborted on the first bad `GET /v1/jobs/{id}`. That exception reaches
`ManagedDatabaseClient._request_with_retry`, which classifies a 502/503 as
transient and re-invokes THE WHOLE OPERATION for every non-append mode -- so a
single blipped status check discards a healthy running job and submits a second
load against a table whose write lock the first one still holds. 409
RESOURCE_LOCKED, which is also transient, and round it goes.
Worse than the behaviour it replaces, not better: before, a lost request took with
it a load that was genuinely being torn down. Now the first job is neither going
away nor releasing anything, and a load polls for up to `_LOAD_JOB_TIMEOUT_S`, so
there are hundreds of status checks where there used to be one request -- every
one a fresh chance to trigger the collision. Exposure up, collision from
incidental to certain.
So the poll tolerates a failed status check and keeps waiting, bounded by
consecutive failures rather than by the first one. An isolated 502 is noise; a run
of them means the API is gone and there is nothing to wait for. The poll's own
deadline bounds the total wait either way.
THE JOB ID IS NOW RETURNED. The docstring claimed the id removed `append`'s "did
it land?" ambiguity, which was not true as implemented: it was a local, discarded
on every failure path, and never reached the caller. `LoadManagedTableResult`
carries it now, for the same reason `CreateIndexResult` does. `append` stays
non-retryable -- knowing the id makes the question answerable, it does not make a
blind re-submission safe, and that call belongs to the caller. The docstring says
that instead of the stronger thing.
`result` is read with `getattr(..., "actual_instance", ...)`, matching the index
path rather than silently disagreeing with it: the strict read raised a bare
AttributeError if the model ever arrived unwrapped.
Did NOT add a strict isinstance check on the inline response. The index path can
assert its inline type because it names it positively first; here the inline shape
is read duck-typed and callers and tests rely on that, so asserting it narrows an
interface this change has no business narrowing. Only the job branch is
type-checked.
Import order fixed for ruff's I rule; `ruff check` clean.
149 passed.
* fix(load): measure poll-error tolerance in time, and name the job when giving up
Two review points, both on paths where the caller is left not knowing whether the
load landed.
TOLERANCE WAS A COUNT, WHICH MEANS NOTHING ON ITS OWN. Five consecutive failures
at the load's 2s interval is about eight seconds, and the thing being survived is a
gateway blip: a rolling restart or a load balancer reconverging serves 502s for
longer than that. Past it the poll aborted, the caller's retry re-submitted the
load, and the first job was still holding the table -- the door this tolerance
exists to close, just narrower than before.
A count is also the wrong unit, because what it means in wall-clock depends on
whichever `interval_s` the caller passed, and callers differ. Now a time bound:
give up after 120s of CONTINUOUS failure, reset on any success, still bounded by
the poll's own deadline. Failed checks also back off to 15s rather than retrying
every 2s -- whatever is serving 502s does not need the extra traffic.
THE GIVE-UP PATHS DROPPED THE JOB ID. `api_error_message(e)` is the gateway's
message ("502: Bad Gateway"), and the failed-job raise preferred the server's
message, which need not mention the id. Those are exactly the two paths where the
answer is unknown and the id is the only handle for asking. Both name the job now,
matching what `_poll_job`'s own TimeoutError already did.
The test for the tolerance drives a FAKE CLOCK advanced by the sleeps. With
`sleep` patched to a no-op and a real clock, it waited out the whole grace period
in wall time -- 120 seconds of a spinning loop to assert one message, and it
slowed the suite from 0.5s to 121s before I noticed. A test measuring a time bound
has to own the clock.
152 passed in 0.53s. `ruff check` clean on the touched files.1 parent f98c313 commit d099390
4 files changed
Lines changed: 459 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
10 | 14 | | |
11 | 15 | | |
12 | 16 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
| 28 | + | |
28 | 29 | | |
29 | 30 | | |
30 | 31 | | |
| |||
81 | 82 | | |
82 | 83 | | |
83 | 84 | | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
84 | 116 | | |
85 | 117 | | |
86 | 118 | | |
| |||
418 | 450 | | |
419 | 451 | | |
420 | 452 | | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
421 | 466 | | |
422 | 467 | | |
423 | 468 | | |
424 | 469 | | |
| 470 | + | |
| 471 | + | |
425 | 472 | | |
426 | 473 | | |
427 | 474 | | |
| |||
432 | 479 | | |
433 | 480 | | |
434 | 481 | | |
| 482 | + | |
| 483 | + | |
| 484 | + | |
| 485 | + | |
| 486 | + | |
| 487 | + | |
| 488 | + | |
| 489 | + | |
435 | 490 | | |
436 | 491 | | |
437 | 492 | | |
438 | 493 | | |
439 | 494 | | |
440 | 495 | | |
| 496 | + | |
441 | 497 | | |
442 | 498 | | |
443 | 499 | | |
| |||
880 | 936 | | |
881 | 937 | | |
882 | 938 | | |
| 939 | + | |
| 940 | + | |
| 941 | + | |
883 | 942 | | |
884 | 943 | | |
885 | 944 | | |
| 945 | + | |
| 946 | + | |
886 | 947 | | |
887 | | - | |
| 948 | + | |
| 949 | + | |
| 950 | + | |
| 951 | + | |
| 952 | + | |
| 953 | + | |
| 954 | + | |
| 955 | + | |
| 956 | + | |
| 957 | + | |
| 958 | + | |
| 959 | + | |
| 960 | + | |
| 961 | + | |
| 962 | + | |
| 963 | + | |
| 964 | + | |
| 965 | + | |
| 966 | + | |
| 967 | + | |
| 968 | + | |
| 969 | + | |
| 970 | + | |
| 971 | + | |
| 972 | + | |
888 | 973 | | |
889 | 974 | | |
890 | 975 | | |
| |||
893 | 978 | | |
894 | 979 | | |
895 | 980 | | |
| 981 | + | |
| 982 | + | |
| 983 | + | |
| 984 | + | |
| 985 | + | |
| 986 | + | |
| 987 | + | |
| 988 | + | |
| 989 | + | |
| 990 | + | |
| 991 | + | |
| 992 | + | |
| 993 | + | |
| 994 | + | |
| 995 | + | |
| 996 | + | |
| 997 | + | |
| 998 | + | |
| 999 | + | |
| 1000 | + | |
| 1001 | + | |
| 1002 | + | |
| 1003 | + | |
| 1004 | + | |
| 1005 | + | |
| 1006 | + | |
| 1007 | + | |
| 1008 | + | |
| 1009 | + | |
| 1010 | + | |
| 1011 | + | |
| 1012 | + | |
| 1013 | + | |
896 | 1014 | | |
897 | 1015 | | |
898 | 1016 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
83 | 83 | | |
84 | 84 | | |
85 | 85 | | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
86 | 91 | | |
87 | 92 | | |
88 | 93 | | |
| |||
0 commit comments