mlevkov commented on PR #3797: URL: https://github.com/apache/iggy/pull/3797#issuecomment-5364604811
Holding this pending a decision on #3795, because `af9ce9548` (#3855) removed its justification. The comment this PR adds says the callback can block "while its bounded forwarding channel is full". On master `c0c74931b` that channel is `flume::unbounded()` (`runtime/src/source.rs:625`), so the send never parks. `handle_messages` also awaits the batch-result oneshot immediately after the callback returns (`sdk/src/source.rs:278`), which yields the worker at that point anyway, so `block_in_place` buys nothing on top of it. The backpressure this protects against exists only if #3795's bounded channel lands, and I have argued over there that the bound is no longer needed. The `SendCallback` signature also gained `batch_id`, which is the conflict showing here. Separately, the discipline this PR was arguing for has already landed in the direction that genuinely blocks: the runtime invokes `batch_result_callback` inside `tokio::task::spawn_blocking` (`runtime/src/source.rs:559`) because the plugin runs its async `on_batch_result` to completion inside that FFI call. So my read is that this closes along with #3795 unless you want the bound, and I am happy to reopen if the bounded channel comes back. Should #3796 close too, or be reframed around the 30s timeout case I described in #3795? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
