[Rust] Multiplexed stream ingest followups#426
Conversation
|
@elenagaljak-db thanks for the review, i've decided to move the ack callback to another PR to not mix contexts here. I've pasted your comments regarding the ack callback on that PR and resolved the ones here |
4ead0b6 to
e3f3f1e
Compare
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
e3f3f1e to
dae8ddf
Compare
| "MultiplexedStream poisoned due to sub-stream failure" | ||
| ); | ||
|
|
||
| let _admission = self.admission.write().await; |
There was a problem hiding this comment.
Not sure if we discussed this before.
Since we hold this lock across join_all(...flush()), an ingest already parked at admission.read().await in enqueue_reserved_admitted cannot acquire the read lock until the whole flush finishes, so it stays blocked for that window before it sees is_closed and errors, and because it also holds sync_mutex, other ingests on that sub-stream queue behind it.
The write lock maybe only needs to be held long enough to drain readers that are mid-enqueue, not for the whole flush. Could we take and immediately drop it as a barrier after setting is_closed, then run the flush and signal_shutdown() unlocked? Any reader that acquires the read lock afterward fails check_closed() anyway, so nothing new is admitted.
There was a problem hiding this comment.
make sense to not block ingests if stream is mid close/failure and return error. Fixed
|
|
||
| #[cfg(feature = "testing")] | ||
| pub(crate) fn capacity_wait_timeout(&self) -> std::time::Duration { | ||
| std::time::Duration::from_millis(self.options.flush_timeout_ms) |
There was a problem hiding this comment.
Do you think it would make sense to have a separate knob for this timeout other than the flush one?
There was a problem hiding this comment.
I added a seperate timeout variable, but didn't make it configureable for the clients. My thinking is that in the future we want the mux stream automatically try the next stream if the first one is full, so it will become an implementation detail the customer doesn't need to worry about. WDYT?
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
Signed-off-by: danilo-najkov-db <danilo.najkov@databricks.com>
|
ill wait for #523 to be merged so I can fix the tests |
What changes are proposed in this pull request?
This fixes the
MultiplexedStreampoison/admission race and makes capacity waits bounded.The new ingest flow is:
The poison path acquires the mux admission write lock before flushing sub-streams, so no record can be admitted between the final closed check and the poison flush.
ZerobusStreamretains ownership of its internal ordering lock throughenqueue_reserved_admitted; the mux supplies only the admission check. Capacity reservations are opaque and cannot expose the underlying semaphore implementation.Capacity waits are bounded by the existing
flush_timeout_msoption. If acknowledgments never free capacity,ingest_record/ingest_recordsreturnZerobusError::ConnectionTimeoutinstead of waiting indefinitely. A capacity timeout does not poison an otherwise healthy mux.How is this tested?
ConnectionTimeoutafter the configured timeout.cargo fmt --all --checkcargo clippy -p databricks-zerobus-ingest-sdk --all-targets -- -D warnings