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

Misc improvements to cluster_controller/logs_controller #2570

Merged
merged 2 commits into from
Jan 30, 2025
Merged

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Jan 28, 2025

Copy link

github-actions bot commented Jan 28, 2025

Test Results

  7 files  ±0    7 suites  ±0   3m 25s ⏱️ - 1m 27s
 47 tests ±0   45 ✅ +1  1 💤 ±0  0 ❌ ±0  1 🔥  - 1 
182 runs  ±0  178 ✅ +1  3 💤 ±0  0 ❌ ±0  1 🔥  - 1 

For more details on these errors, see this check.

Results for commit 27e8e07. ± Comparison against base commit c61c551.

♻️ This comment has been updated with latest results.

@pcholakov
Copy link
Contributor

Still very similar experience to before - about 10 min after the end of the test, all nodes remain unresponsive to ingress requests. Test results at: #2565 (comment)

@AhmedSoliman
Copy link
Contributor Author

I'll revisit some of the logging again before the releasae.

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 improvements for the cluster controller. LGTM. +1 for merging.

crates/admin/src/cluster_controller/logs_controller.rs Outdated Show resolved Hide resolved
crates/admin/src/cluster_controller/logs_controller.rs Outdated Show resolved Hide resolved
tokio::time::sleep(delay).await;
}

match bifrost.admin().seal(log_id, segment_index).await {
Ok(sealed_segment) => {
if sealed_segment.tail.is_sealed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is sealed_segment.tail.is_sealed() reliable or can it return stale information?

Copy link
Contributor Author

@AhmedSoliman AhmedSoliman Jan 30, 2025

Choose a reason for hiding this comment

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

if seal returns Ok, the segment must be sealed.

For futures where we want to observe if the are too slow or overdue, this extension trait adds the ability to log with automatic exponential backoff on a certain tracing level, then optionally upgrade the level and reports once it crosses the "overdue" threshold.

## Usage:
```rust

use restate_futures_util::overdue::OverdueLoggingExt;

let result = my_future
  .log_slow_after(
    Duration::from_secs(1),
    tracing::Level::INFO,
    "Doing something important.",
  )
  // optional
  .with_overdue(
    Duration::from_secs(30),
    tracing::Level::WARN
).await;

// prints
// INFO "[slow] Doing something important. elapsed=1s");
// INFO "[slow] Doing something important. elapsed=2s");
// INFO "[slow] Doing something important. elapsed=4s");
// INFO "[slow] Doing something important. elapsed=8s");
// ...
// WARN "[overdue] Doing something important. elapsed=30s");
// WARN "[completed] Doing something important. elapsed=35.9s");

```
Some of the debug messages will likely be changed to trace before the release cut off date
@AhmedSoliman AhmedSoliman merged commit 27e8e07 into main Jan 30, 2025
25 of 29 checks passed
@AhmedSoliman AhmedSoliman deleted the pr2570 branch January 30, 2025 12:12
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.

3 participants