[bifrost] Introduce a new LogChainWatcher for efficient subscriptions to chain mutations#4953
[bifrost] Introduce a new LogChainWatcher for efficient subscriptions to chain mutations#4953MohamedBassem wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d48ce8b23d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5067c6c89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let wait_for_writable_segment = bifrost_inner.log_chain_watcher().subscribe( | ||
| log_id, | ||
| crate::log_chain_watcher::ChainCondition::WritableSegmentAfter(sealed_segment), | ||
| ); |
There was a problem hiding this comment.
If a replacement segment appears and is sealed again before the watcher handles the intermediate writable metadata version, this subscription never fires: WritableSegmentAfter requires non_special_tail().tail_lsn.is_none() in log_chain_watcher.rs:233-238, so the current seal-marker state is false. However this loop treats any later tail with tail_lsn.is_none() as progress and returns it at appender.rs:367-384; missing that update leaves the appender asleep until the retry timer before it can advance recovery to the latest segment, extending write unavailability by the configured backoff. Please subscribe to a condition that also wakes on tail-index advancement.
Useful? React with 👍 / 👎.
MohamedBassem
force-pushed
the
fix/seal-watcher
branch
from
cf6760a to
63f06bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f06bbd95
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
MohamedBassem
force-pushed
the
fix/seal-watcher
branch
2 times, most recently
from
2e2f0fa to
7dcdbf0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dcdbf0225
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.subscriptions | ||
| .entry(sub.log_id) | ||
| .or_default() | ||
| .push_back(sub); |
There was a problem hiding this comment.
When an appender's tokio::select! takes the backoff sleep branch before this watcher drains the inbound request, the SubscriptionHandle is dropped and this oneshot::Sender is already closed, but it still gets queued here. During a sealed-log outage with many appenders timing out, the watcher can accumulate and later scan/attempt to notify dead waiters until a config change or probabilistic reap happens, adding avoidable work on the task that should wake the remaining live appenders; please skip closed senders before registering them.
Useful? React with 👍 / 👎.
… to chain mutations Currently, there's no efficient way for subscribing to certain mutations to the log chains. Callsites (like appender::on_sealed_loglet) end up subscribing to any logs config mutation and loop if it's irrelevant. This results into frequent unnecessary wakeups across the different callsites. This commit introduces a new background task called LogChainWatcher which allows for targeted and efficient subscriptions to log chain mutations. It creates a single subscriber to logs config mutations and notifies the watchers if the current chain condition matches their subscription. As a start, the watcher allows you to subscribe to two events: A certain segment getting sealed: Which will be used in a later PR to abort the appender's in-flight append if the segment it's writing to gets sealed. Chain becoming writable: which will be used by the appender to efficiently wait for a new writable segment to appear instead of subscribing to all mutations.
MohamedBassem
force-pushed
the
fix/seal-watcher
branch
from
7dcdbf0 to
ea0f71d
Compare
|
Discussed this offline and agreed that there could be a simpler path here that @MohamedBassem will explore. |
|
Moving this back to a draft as I need to test the new implementation first. |
2 participants
Currently, there's no efficient way for subscribing to certain mutations to the log chains. Callsites (like
appender::on_sealed_loglet) end up subscribing to any logs config mutation and loop if it's irrelevant. This results into frequent unnecessary wakeups across the different callsites.This PR introduces a new background task called
LogChainWatcherwhich allows for targeted and efficient subscriptions to log chain mutations. It creates a single subscriber to logs config mutations and notifies the watchers if the current chain condition matches their subscription.As a start, the watcher allows you to subscribe to two events:
Implementation notes:
#[cfg(test)]stuff in the watcher to make the tests more reliable (and avoid timed waits for non ready subs). Happy to revisit it if it feels ugly.