GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [connectors] Retry DynamoDB writes through throttling and report health by bestouff · Pull Request #7736 · feldera/feldera Conversation
|
| Error | Action |
|---|---|
| Throttling, timeout, network, 5xx, transient or stale credentials | Retry, budgeted by max_retries |
| Transaction cancelled by a transient item error, or by a reason this connector does not know | Retry the rest |
| Validation, missing table, denied access, bad config | Drop the chunk at once |
- One
max_retriesbudget covers unprocessed items, transient cancellations and retryable request errors in both write modes.nullretries until DynamoDB recovers. - The retryable code lists come from
aws-runtime, which the DynamoDB SDK already pulls in, so they cannot drift from the SDK's own retry behaviour.UnrecognizedClientExceptionandExpiredTokenExceptionare retried too, since the SDK's identity cache refreshes the credentials on a later attempt. - A credential-provider failure comes as a dispatch failure with no retry kind, so the source chain decides: only
InvalidConfigurationis permanent. - A chunk that gives up no longer drops the rest of its step. The worker holds the errors of finished chunks until the batch ends, every flush still sends its own chunk, and the rows that landed are counted whether or not a chunk of the step failed.
- Diagnostics keep the service's code, message, throttling reasons, cancellation reasons and request id, and drop the raw HTTP response.
Health and metrics
The chunk writers of one endpoint share a RetryHealth that publishes the sink:
| State | When |
|---|---|
| Unchanged | A chunk has retried for less than 10 seconds: a retry under load is no outage |
| Unhealthy, with the cause | A chunk has retried for 10 seconds or more |
| Unhealthy, with the failure | A chunk gave up; this outranks the cause of a retry in progress |
| Healthy | No chunk retries and a chunk has landed |
A pipeline stop releases the count without a report. New gauge dynamodb_output_retrying_chunks shows the chunks retrying now; dynamodb_output_retries_total now counts retries as they start; dynamodb_output_transact_write_failures_total counts every failed request, retried or not.
Tests
- Unit tests drive a wiremock DynamoDB through throttling, partial success, cancellation reasons, credential failures, the exact budget, the health threshold, concurrent retries, a failure beside a retry loop and shutdown. wiremock is an optional dependency under the
testingfeature. - Five connector-suite cases run the real controller: health through a throttling outage, a stop during an outage that never ends, and a permanent failure in the middle of a step, the last two in both write modes.
- Fourteen reverts (batch retry, classifier, health report, retry count, guard drop, transactional fallback, the threshold, the failure precedence, the reclassified codes, the flush that kept bailing, the skipped output counters, and more) each make the named test fail.
- No manual run against AWS: DynamoDB Local never throttles, so the throttling paths are covered by the wiremock server and the controller suite only.
Docs: max_retries description and a new "Retries and health" section in the DynamoDB sink page, plus a changelog entry.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
1cf852f to
bb7d3a6
Compare
|
I asked claude to investigate specific areas of this PR. It found some issues. Some of them need your judgement call. The most important one is the data loss issue (section 3). Apparently this is not a new behavior, but it seems wrong, and needs fixing in this or separate PR.
One permanent failure silently drops the rest of the worker's step (confirmed end to end). This is old behavior in batch mode. The PR exposes transactional mode to it, because that mode used to retry permanent errors forever (stalling instead of losing data).
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| /// The errors held since the last batch end, joined into one. | ||
| fn take_write_errors(&mut self) -> Option<anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pitfall (medium): write_errors has no bound now that a mid-step flush no longer bails. If every chunk fails permanently (for example a missing table or denied access), every chunk of the step sends its request, and each failure is joined into one string here and logged with its own warn!. I ran a QA test with 2000 chunks of 1 item against ResourceNotFoundException: 2000 requests and a 434 KB error message. A step of 1M rows at batch_size=25 would make about 9 MB, sent to output_transport_error and the logs. Could you dedupe the errors by cause, or keep the first N plus a count ("…and 39 998 more")? Stopping the step after the first permanent error would also avoid the pointless requests.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // The credentials the request carried are stale or not yet valid: a | ||
| // rotated role, a key that is still propagating, a token past its | ||
| // expiry. The SDK's identity cache refreshes them on a later attempt. | ||
| "UnrecognizedClientException", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pitfall (medium): DynamoDB also returns UnrecognizedClientException for a static aws_access_key_id that is simply wrong (a typo or a deleted key), not only for a stale one. With this list, that misconfiguration retries for the whole budget (about a minute by default) and forever with max_retries: null. It only looks like a slow throttle (the cause shows after 10 s), and nothing drops it at once. That contradicts the docs, which say "denied access, or an invalid credential configuration, discards the chunk at once". Could you retry these codes only when credentials come from a refreshing provider (not static keys), or cap them at a few retries whatever the budget? At minimum, document the behaviour.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| ); | ||
| assert_eq!(gate.entered(), 1); | ||
| assert_eq!(harness.metric("dynamodb_output_retrying_chunks"), 1.0); | ||
| assert!(harness.publisher.reports().is_empty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flaky (medium): This assertion races a 500 ms wall-clock timer. The overdue timeout starts when the chunk enters its delay, before gate.entered() returns. If the test thread is descheduled for more than 500 ms between that recv and this check (as happens on a loaded CI runner), the report has already landed and the test fails. It passed 15/15 for me under CPU load, but the window is real. The unit test a_retry_counts_without_a_report already covers "no report before the threshold", so I suggest dropping this line (and making the poll loop below do the work). Alternatively, use a threshold large enough that a stall can't reach it.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
Review summary: DynamoDB retry classification + health What I ran
Gates
Issues
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
A `BatchWriteItem` request that failed with a service error, throttling included, was returned at once: the chunk was dropped as soon as the AWS SDK's own three attempts ran out. Only unprocessed items in a successful response were retried. The transactional writer retried every error, the permanent ones too, forever when `max_retries` was null. Neither reported the health of the sink. Classify a failed request before deciding what to do with it: | Error | Action | |---------------------------------------------------------|---------------| | Throttling, timeout, network, 5xx, transient credentials | Retry, budgeted | | Transaction cancelled by a transient item error | Retry the rest | | Validation, missing table, denied access, bad config | Drop at once | One `max_retries` budget covers unprocessed items, transient cancellations and retryable request errors in both write modes. A credential-provider failure comes as a dispatch failure with no retry kind, so the source chain decides: only `InvalidConfiguration` is permanent. Diagnostics keep the service's code, message, throttling reasons and request id, and drop the raw HTTP response. The chunk writers of one endpoint share a `RetryHealth` that publishes the sink unhealthy while a chunk retries, with the cause, unhealthy after a chunk gives up, and healthy once no chunk retries and a chunk has landed. A pipeline stop releases the count without a report. The gauge `dynamodb_output_retrying_chunks` shows the chunks retrying now, and `dynamodb_output_retries_total` counts retries as they start. The retry loops get a delay seam, and the tests drive a wiremock DynamoDB through throttling, partial success, cancellation reasons, credential failures, the exact budget, concurrent retries and shutdown. Three connector-suite cases run the real controller: health through a throttling outage, and a stop during an outage that never ends, in both write modes. With the batch retry, the classifier or the health report reverted, the matching tests fail. Closes #7595. Signed-off-by: Xavier Bestel <[email protected]>
The throttling and transient error code lists were copied from `aws-runtime`, which is already in the dependency graph of the DynamoDB SDK and exports both lists. A copy drifts from the SDK's own retry behaviour when the SDK extends its list. Depend on the crate under `with-dynamodb`, like the other SDK crates the classifier names, and read the lists from it. Signed-off-by: Xavier Bestel <[email protected]>
Transaction cancellations lost the item reasons that explain why a write was rejected, and response errors still included the raw HTTP response. Keep cancellation codes and messages, and describe response errors using their source chain, status, and request ID. Omit returned item data. Hold the controller test's mock outage until it has checked health and retry metrics. Compile the no-op health publisher and its import only in test builds, so production lint fixes cannot break the connector suites. Add regression coverage for cancellation details and response formatting. Validate with 53 DynamoDB unit tests, 47 connector integration tests against DynamoDB Local, five recovery-test repetitions, and all-target Clippy with warnings denied. Signed-off-by: Xavier Bestel <[email protected]>
…h late A chunk that gave up failed the worker's next flush before that flush sent its own chunk, and the encode loop stopped at the failed push. The rest of the step was never sent and never counted, while the error named only the chunk that failed. Hold the errors of finished chunks in the worker until the batch ends, so that a flush always sends its chunk, and count the rows that landed in a step whether or not a chunk of it gave up. Report the endpoint unhealthy once a chunk has retried for ten seconds rather than at its first retry: a partial response under load is no outage. The report comes during the wait when the threshold falls inside it. A failure that no later chunk has cleared outranks the cause of a retry in progress, since lost records matter more. Retry `UnrecognizedClientException` and `ExpiredTokenException`, which the SDK's identity cache clears on a later attempt, and retry a transaction cancelled by a reason this connector does not know; only `ValidationError` ends a transaction for good. Describe `transact_write_failures_total` as every failed request, retried or not. Tests cover the mid-step failure in both write modes through the controller, the threshold, the failure precedence, the reclassified codes, two workers sharing one health tracker, and the errors a stop may report. Signed-off-by: Xavier Bestel <[email protected]>
ryzhyk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for resolving all the issues.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
Uh oh!
There was an error while loading. Please reload this page.
|
Verdict: likely flaky test, not a bug in this PR's logic. Moderate confidence — I could not reproduce it. Evidence
The Next step: filed #7828 with the details. Re-queueing is likely to pass — if it fails the same way again, it's worth tightening the 500ms margin in that test rather than chasing the connector code. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Re-failure of #7828 — same flaky test, fix pushed.
This test races a 500ms real-wall-clock Fix: widened the threshold from 500ms to 3s on Take it with: Compare: issue-7595...fred/observe-7736-41850195763 Logged the recurrence on #7828 as well. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…rns_unhealthy_after_the_threshold The test asserted that reports() was still empty right after the chunk entered its retry wait, racing a 500ms real-wall-clock threshold. On a busy CI runner the test thread can lose the scheduler for that long before it gets to the assertion, so the health report had already landed. Raise the threshold to 3s to give the check a realistic margin; the test's own 10s bound on the opposite wait still leaves plenty of room for the report to show up. Seen twice in the merge queue for #7736 (runs 41750935279 and 41850195763), tracked in #7828. Signed-off-by: mythical-fred-oss[bot] <319348225+mythical-fred-oss[bot]@users.noreply.github.com>
…d one `MockServer::start()` hands out servers from a pool and recycles them when a test drops its server. A chunk writer that a failed test leaves behind on the shared runtime can send one more request, which then lands on the recycled server of whichever test picked it up next, and that test counts it as its own. In CI, `the_retry_budget_is_exact` saw two requests instead of one right after `a_long_retry_loop_turns_unhealthy_after_the_threshold` failed. `MockServer::builder().start()` returns a server that is not pooled, so a stray request only ever reaches the server of the test that made it. Reproduced with a panic injected after the chunk entered its retry and the released task delayed by 100 ms: 5 of 10 runs broke another retry test with the pooled server, 0 of 10 with this change. Refs #7828.
The timer that reports a long retry loop unhealthy starts at the chunk's first retry, which is after the test calls `write_batch`. So an instant taken before that call bounds the timer: while less than the threshold has passed since then, no report can exist yet, and the assertion holds by construction. A runner that stalls past the threshold skips the check instead of failing it, so the test no longer races a wall clock. Refs #7828.
Uh oh!
There was an error while loading. Please reload this page.
…rns_unhealthy_after_the_threshold The test asserted that reports() was still empty right after the chunk entered its retry wait, racing a 500ms real-wall-clock threshold. On a busy CI runner the test thread can lose the scheduler for that long before it gets to the assertion, so the health report had already landed. Raise the threshold to 3s to give the check a realistic margin; the test's own 10s bound on the opposite wait still leaves plenty of room for the report to show up. Seen twice in the merge queue for feldera#7736 (runs 41750935279 and 41850195763), tracked in feldera#7828. Signed-off-by: mythical-fred-oss[bot] <319348225+mythical-fred-oss[bot]@users.noreply.github.com>
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
[connectors] DynamoDB: retry on throttling errorsUh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.