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] Sequencer gives priority to GetSequencerState requests #2672

Merged
merged 6 commits into from
Feb 8, 2025

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Feb 7, 2025

Those requests are really important to be handled quickly, we are doing two changes in this PR:

  • Making expensive to run GetSequencerState operations not block the network loop
  • Give priority to handling those request vs appends
// intentionally empty

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

github-actions bot commented Feb 7, 2025

Test Results

  7 files  ±0    7 suites  ±0   3m 29s ⏱️ - 1m 7s
 45 tests  - 2   44 ✅  - 2  1 💤 ±0  0 ❌ ±0 
174 runs   - 8  171 ✅  - 8  3 💤 ±0  0 ❌ ±0 

Results for commit 9345693. ± Comparison against base commit 765636b.

This pull request removes 2 tests.
dev.restate.sdktesting.tests.AwaitTimeout ‑ timeout(Client)
dev.restate.sdktesting.tests.RawHandler ‑ rawHandler(Client)

♻️ 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.

LGTM. +1 for merging :-)

This enables nodes to maintain concurrent connections across different actual TCP connections to increase message processing concurrency. This is controlled by a new configuration option in `[networking]` section.

This also tags a few operations with `tokio::task::unconstrained` to reduce unnecessary coop-driven yields that happen at some hot-paths.

```
// intentionally empty
```
Introducing histograms and per-message counters. Histograms for processing duration per message are only enabled in debug builds but this decision can change if we found the need to use them for release build

```
// intentionally empty
```
Debug assertions are not meant to be used in release builds, most crates use `cfg(debug_assertions)` as a proxy to detect whether they are built in debug mode or not, this include `tracing` among many others. The side effect of having debug_assertions enabled is (a) performance penalty and (b) TRACE level being enabled in release build because `tracing` can't respect the release max logging filter feature flag if it doesn't know we are building in release mode.

```
// intentionally empty
```
Among other small bits and bobs

```
intentionally empty
```
Those requests are really important to be handled quickly, we are doing two changes in this PR:
- Making expensive to run GetSequencerState operations not block the network loop
- Give priority to handling those request vs appends

```
// intentionally empty
```
@AhmedSoliman AhmedSoliman merged commit 494c7f8 into main Feb 8, 2025
33 of 34 checks passed
@AhmedSoliman AhmedSoliman deleted the pr2672 branch February 8, 2025 12:47
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