Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bifrost] Basic BifrostAdmin interface #1753

Merged
merged 5 commits into from
Jul 26, 2024
Merged

[Bifrost] Basic BifrostAdmin interface #1753

merged 5 commits into from
Jul 26, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Jul 25, 2024

Copy link

github-actions bot commented Jul 25, 2024

Test Results

102 files  ±0  102 suites  ±0   22m 54s ⏱️ -28s
 84 tests ±0   84 ✅ ±0  0 💤 ±0  0 ❌ ±0 
217 runs  ±0  217 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e3bfec7. ± Comparison against base commit 357c317.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @AhmedSoliman. LGTM. +1 for merging :-)

crates/bifrost/src/bifrost_admin.rs Outdated Show resolved Hide resolved
crates/bifrost/src/read_stream.rs Show resolved Hide resolved
Comment on lines +176 to +177
/// Deletes the key-value pair for the given key following the provided precondition. If the
/// precondition is not met, then the operation returns a [`WriteError::PreconditionViolation`].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these Rustdocs 🙏

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
```
This allows correct handling of sealing empty loglets and support for future reconfiguration operations.
It now hosts `trim` and introduces the ability to seal and extend the chain with a pre-determined loglet params
@AhmedSoliman AhmedSoliman merged commit e3bfec7 into main Jul 26, 2024
13 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1753 branch July 26, 2024 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants