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

[TaskCenter][4/n] Remove TaskCenter/Metadata references from Bifrost #2344

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Nov 21, 2024

Also, exercises in_tc() in some tests. Those will be swapped with restate_core::test in next PRs.

Example of impact:

  let bifrost = node_env
      .tc
      .run_in_scope(
          "bifrost init",
          None,
          Bifrost::init_in_memory(node_env.metadata.clone()),
      )
      .await;

Becomes:

  let bifrost = Bifrost::init_in_memory().in_tc(&node_env.tc).await;

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

github-actions bot commented Nov 21, 2024

Test Results

  7 files  ±0    7 suites  ±0   4m 21s ⏱️ -9s
 47 tests ±0   46 ✅ ±0  1 💤 ±0  0 ❌ ±0 
182 runs  ±0  179 ✅ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit 9b5b5ca. ± Comparison against base commit c19fcdf.

♻️ 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. Looking forward to the macro that was teased in this chapter :-)

- Adds `in_tc(&TaskCenter)` and `in_current_tc()` to scope a future to run in a task-center if it'll be dispatched as a task
- Introduces a root task (TaskId(0)) in TaskCenter that's used as a default parent/source of task context
- Removes unnecessary args from block_on, run_in_scope_sync. (some of those will be changed in future PRs)
- Introduces `Metadata::current()` and `Metadata::with_current(F)`
- Improvements to the future extensions (fixing root task context propagation)
- A round of removals of explicit task-center reference passing
- `try_set_global_metadata` is now an associated function

## Example

Before:
```rust
  task_center().spawn_child(
      TaskKind::RpcServer,
      "metadata-store-grpc",
      None,
      async move {
          net_util::run_hyper_server(
              &bind_address,
              service,
              "metadata-store-grpc",
              || health_status.update(MetadataServerStatus::Ready),
              || health_status.update(MetadataServerStatus::Unknown),
          )
          .await?;
          Ok(())
      },
  )?;
```

After:
```rust
  TaskCenter::spawn_child(TaskKind::RpcServer, "metadata-store-grpc", async move {
      net_util::run_hyper_server(
          &bind_address,
          service,
          "metadata-store-grpc",
          || health_status.update(MetadataServerStatus::Ready),
          || health_status.update(MetadataServerStatus::Unknown),
      )
      .await?;
      Ok(())
  })?;
```
Also, exercises `in_tc()` in some tests. Those will be swapped with `restate_core::test` in next PRs.

Example of impact:
```rust
  let bifrost = node_env
      .tc
      .run_in_scope(
          "bifrost init",
          None,
          Bifrost::init_in_memory(node_env.metadata.clone()),
      )
      .await;
```

Becomes:
```rust
  let bifrost = Bifrost::init_in_memory().in_tc(&node_env.tc).await;
```
@AhmedSoliman AhmedSoliman merged commit d0f01b7 into main Nov 25, 2024
24 checks passed
@AhmedSoliman AhmedSoliman deleted the pr2344 branch November 25, 2024 19:10
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