feat(connectors): add source batch acknowledgments - #3855
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3855 +/- ##
=============================================
- Coverage 83.67% 68.90% -14.77%
+ Complexity 1358 1339 -19
=============================================
Files 1212 1209 -3
Lines 165138 139252 -25886
Branches 132612 107565 -25047
=============================================
- Hits 138181 95957 -42224
- Misses 23308 39625 +16317
- Partials 3649 3670 +21
🚀 New features to boost your workflow:
|
|
/ready |
|
@hubcio This PR is ready for review. |
hubcio
left a comment
There was a problem hiding this comment.
i actually had comments ready yesterday, just as pending, forgot to post them. here a few things that don't sit on changed lines:
- no test exercises the failure path - nothing forces a send or state-save failure and checks that the nack arrives and the state file stays put. and no integration test ever waits for a second batch, so a handshake that wedges after batch 1 still passes the suite.
manager/source.rs:150comment "close FFI (stops callbacks)" is stale now - close doesn't stop the runtime->plugin result callback, it guarantees it fails.- the connector authoring docs and templates don't mention
on_batch_result, so the next source written from them lands with the no-op default too.
|
ignore failing java examples, we'll check it |
|
Thanks for the detailed review. I agree with the issues you identified and will address the ABI mismatch, shutdown and timeout handling, callback error propagation, NACK backoff, docs/templates, and missing regression tests. For processing failures, I propose NACK with capped backoff, eventually stopping the connector without ACKing instead of losing data. For partial producer failures, I’ll report committed/failed counts and retry only the failed tail before NACKing. As agreed in #3635, I’ll keep PostgreSQL migration in the follow-up PR, use a small generic source to test this contract, and narrow the README claims until PostgreSQL is migrated. Does this look good? |
|
@rohankumardubey looks good. |
|
looks like test is failing @rohankumardubey https://github.com/apache/iggy/actions/runs/31840994243/job/94897663656?pr=3855 |
The test was stopping only node 0 while running against a three-node cluster. After the first failure, the producer failed over to another node, so the test timed out waiting for a second failure. I’ve changed this regression to use a single-node cluster, which keeps the outage deterministic. I’ll push the fix after the local checks are complete. |
|
@rohankumardubey looks good, we'll merge this once github will start working properly again |
Thanks for the review! I’ve already started working on the PostgreSQL follow-up from #3635, including the staged cursor changes and deterministic kill-server regression test we discussed. |
Which issue does this PR address?
Relates to #3635
Rationale
Establishes the generic source acknowledgment contract before migrating PostgreSQL source behavior and adding the kill-server regression test in a follow-up PR.
What changed?
Source plugins could poll another batch without knowing whether the previous batch was delivered, allowing cursors and persisted state to advance after failed sends.
The source FFI now carries batch IDs and supports ACK/NACK results. The SDK permits one in-flight batch, while the runtime ACKs only after both the Iggy send and state persistence succeed. Send failures, timeouts, and state-save failures produce a NACK.
Local Execution
cargo fmt --alliggy_source_batch_resultsymbol is exportedcargo-sort,markdownlint, andtaplowere not available locally