[Bifrost] Read stream support multi-segment logs #1744
Conversation
AhmedSoliman
force-pushed
the
pr1744
branch
2 times, most recently
from
cae8f5f
to
23d9220
Compare
|
Pulling it back from the review queue until e2e is fixed. |
|
This is now ready to review |
There was a problem hiding this comment.
Great piece of work @AhmedSoliman 🚀 Really liking how easy it is to follow the elaborate logic of the LogletReadStream by having the explicit state machine spelled out :-) LGTM. +1 for merging.
| // BifrostInner is dropped last, we can safely lift it's lifetime to 'static as | ||
| // long as we don't leak this externally. External users should not see any `'static` | ||
| // lifetime as a result. | ||
| let bifrost_inner = unsafe { &*Arc::as_ptr(&self.bifrost_inner) }; |
There was a problem hiding this comment.
Do we want to save the atomic increment operation on the Arc if we used instead Arc::clone(&self.bifrost_inner)?
There was a problem hiding this comment.
I'm on the fence, but let's keep it this way because I suspect we will need to use it a little more and I don't want to arc::clone on the critical path of polls.
| this.state.set(State::Terminated); | ||
| return Poll::Ready(Some(Err(ShutdownError.into()))); | ||
| } | ||
| Some(TailState::Open(tail)) => { |
There was a problem hiding this comment.
A loglet will only return Open for a given lsn if it knows that this was safely committed, right?
There was a problem hiding this comment.
Yes. that's the contract.
Loglet wrapper now limits reads if the loglet sealed tail is known
This introduces a new read stream implementation that operates under a multi-segment bifrost world. Notable features include: - Support for reading from multiple segments seamlessly - Reading unsealed segments while watching the tail state to determine the safe boundaries with minimal efficiency loss - Handling of on-going reconfiguration, the stream waits for the loglet to be sealed. - Handles prefix trims on metadata-level when detected (partial support, more on that in follow up PRs) Running bifrost-benchpress read-to-write latency tests show that the new read-stream doesn't introduce any meaningful regression in latency in the the unsealed close-to-tail case (note that P100 should be discarded due to shutdown-related noise) Write-to-read latency: ``` New Old Total records read: 98317 Total records read: 97871 P50: 67.455µs P50: 67.519µs P90: 77.951µs P90: 77.183µs P99: 96.447µs P99: 94.143µs P999: 129.215µs P999: 122.815µs ```
[Bifrost] Read stream support multi-segment logs
This introduces a new read stream implementation that operates under a multi-segment bifrost world. Notable features include:
Running bifrost-benchpress read-to-write latency tests show that the new read-stream doesn't introduce any meaningful regression in latency in the the unsealed close-to-tail case (note that P100 should be discarded due to shutdown-related noise)
Write-to-read latency:
Stack created with Sapling. Best reviewed with ReviewStack.