[Bifrost] BackgroundAppender implementation #1789
Conversation
AhmedSoliman
force-pushed
the
pr1789
branch
2 times, most recently
from
49d5d2d to
7e2e5b7
Compare
Test Results 5 files 5 suites 6m 7s ⏱️ Results for commit 35b10b9. ♻️ This comment has been updated with latest results. |
AhmedSoliman
force-pushed
the
pr1789
branch
3 times, most recently
from
988a3aa to
68e2d74
Compare
| let keys = record.record_keys(); | ||
| StorageCodec::encode(&record, &mut appender.serde_buffer) | ||
| .expect("record serde is infallible"); | ||
| // todo, optimize to avoid copying the payload bytes again | ||
| let payload = Payload::new(appender.serde_buffer.split().freeze()); | ||
| StorageCodec::encode(payload, &mut appender.serde_buffer) | ||
| .expect("record serde is infallible"); |
There was a problem hiding this comment.
Maybe factor out into a function encode_as_payload<T: StorageEncode + HasRecordKeys>(value: &T, ...)? I think I have seen this snippet at a few places.
There was a problem hiding this comment.
Good idea but this code gets removed in a follow-up PR anyway so I wouldn't worry so much about it.
| // What to do if task panics! | ||
| if let Err(e) = task.await { | ||
| warn!( | ||
| ?e, |
There was a problem hiding this comment.
Maybe err so that from the field name is clear that this is the error?
| #[strum(props(OnCancel = "wait", OnError = "log"))] | ||
| BifrostAppender, |
There was a problem hiding this comment.
Since we are awaiting on cancel but do not listen to the task's cancellation token, we assume that this task will always be canceled together with the task that holds the AppenderHandle, right?
There was a problem hiding this comment.
Yes, that's correct. appender handle's description explains this.
Envelope implements a new trait `HasRecordKeys` to support future matching of records based on a `KeyMatcher`. The new appender is the canonical way to write to a single log. It tries to recycle the write buffer (although it doesn't use freelist which would be a potential future improvement) and it defers serialization to unlock the potential for better serialization write buffer management. Appender is created by `bifrost.create_appender(log_id)?` and it's the core abstraction that `BackgroundAppender` builds upon (next PR)
Introduces a new way to enqueue messages to Bifrost. This PR adds `bifrost.create_background_append(...).start()` that starts a background appender. The background appender automatically batches appends and provides flexible control over draining it or auto-destruction when all senders are dropped (see `detach()` on `AppenderHandle`). With this, `ActionEffectHandler` makes use of this new ability and will not block the partition processor loop when writing effects to bifrost.
[Bifrost] BackgroundAppender implementation
Introduces a new way to enqueue messages to Bifrost. This PR adds
bifrost.create_background_append(...).start()that starts a background appender. The background appender automatically batches appends and provides flexible control over draining it or auto-destruction when all senders are dropped (seedetach()onAppenderHandle).With this,
ActionEffectHandlermakes use of this new ability and will not block the partition processor loop when writing effects to bifrost.Stack created with Sapling. Best reviewed with ReviewStack.