-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
grpc: AsyncTaskExecutor and basic server lifecycle #11984
Conversation
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
// return res_obj; | ||
// }); | ||
// | ||
// - TODO: Move this to more standard location inside codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. maybe move to flow
or fdbrpc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if the CI failure is related or not. I'' merge if not related to this PR.
Action(Func&& fn) : fn_(std::forward<Func>(fn)) {} | ||
|
||
void operator()(IThreadPoolReceiver* action) override { | ||
promise_.send(fn_()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will wrap fn() inside try/catch to forward errors in next PR.
CI failure doesn't seem to be this PR related. |
This patch implements `AsyncTaskExecutor` for asynchronous execution of tasks in a separate thread pool. We already have `IThreadPool` however its API is more well suited for bigger tasks. This just provides an easier to use API. There is `AsyncTaskThread` which is similar in nature, but this is not re-wrapping IThreadPool hence has ability to have multiple worker threads. We can potentially replace that with this component by setting `num_threads = 1`. TODO: Move this to `flow/include` instead of here.
This patch has two set of changes: - Whenever a service is registered and removed from server, we need to restart gRPC server. GrpcServer provides some methods that can be used by worker actors so that the life of services registered by them can tied to the life of the worker role itself. - Replace asio::thread_pool with AsyncTaskExecutor both in client and server.
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
@jzhou77 Correctness passed: 20250306-003951-vishesh-7d25265389131d54 compressed=True data_size=40885293 duration=5971889 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:48:08 sanity=False started=100000 stopped=20250306-012759 submitted=20250306-003951 timeout=5400 username=vishesh |
This PR has three changes:
asio::post
orstd::async
. There isAsyncTaskThread
in fdbclient which serves similar purpose, however it doesn't useIThreadPool
hence doesn't set the simulation thread_locals correcly, and also just uses one thread. That should be equivalent of setting num_thread = 1 in this class.asio::thread_pool
.Testing:
Some correctness issues around RockDB. Debugging, but some were unrelated to this. e.g. #11981
Has some basic unit-tests and will keep adding more in subsequent PRs where I'm actually making use of these.
Update:
20250306-003951-vishesh-7d25265389131d54 compressed=True data_size=40885293 duration=5971889 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:48:08 sanity=False started=100000 stopped=20250306-012759 submitted=20250306-003951 timeout=5400 username=vishesh
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branch
ormain
if this is the youngest branch)