8000 Route `set/getFunctionResult` through the planner by csegarragonz · Pull Request #317 · faasm/faabric · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Route set/getFunctionResult through the planner #317

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

Merged
merged 31 commits into from
Jun 21, 2023
Merged

Conversation

csegarragonz
Copy link
Collaborator
@csegarragonz csegarragonz commented Jun 2, 2023

In this PR we move the setting/getting of function results to the planner.

When setting a function (message) result, the local scheduler just sends a request to the planner with the message result. The planner then stores it.

When getting a function result, we query the planner for the message result. Two things could happen:

  1. The message result is recorded in the planner, and we return.
  2. The message result is not recorded in the planner, and thus we have to wait.

In order to avoid blocking the server threads in the planner, we wait on the client side (i.e. worker thread). In situation 2, i.e. if we have asked for a message result and the message is not there, the planner registers the host's interest in the message result, and the worker waits on a result promise. When the result is finally set, the planner will notifiy all hosts that have registered interest in that message result via a FunctionCall.

In the process of changing this I also remove the asynchronous waiting implemented in the HTTP endpoint. The reason being twofold:

  • On the one hand, function execution requests will eventually go through the planner endpoint (and not the worker endpoint), so that piece of code will not be necessary anymore.
  • Given the above, and in the interest of simplifying the get/set result process, I make the HTTP endpoint thread also block while waiting for the result.

In addition, I also clean-up and simplify the base fixtures in tests/util/fixtures.h. I make sure that fixtures that just mimick one component don't unnecessarily inherit from other fixtures. This was creating friction with Faasm's tests, where these base fixtures are also used. This change creates a lot of noise in the diff, but doesn't change any functionality.

@csegarragonz csegarragonz self-assigned this Jun 5, 2023
@csegarragonz csegarragonz mentioned this pull request Jun 5, 2023
45 tasks
@csegarragonz csegarragonz marked this pull request as ready for review June 7, 2023 15:55
@csegarragonz csegarragonz force-pushed the get-set-msg-result branch 2 times, most recently from 69daff7 to 546ba18 Compare June 8, 2023 13:35
@codecov
Copy link
codecov bot commented Jun 8, 2023

Codecov Report

Merging #317 (37c7364) into main (97c8262) will increase coverage by 0.04%.
The diff coverage is 99.36%.

@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   82.42%   82.46%   +0.04%     
==========================================
  Files          97       97              
  Lines        7822     7803      -19     
==========================================
- Hits         6447     6435      -12     
+ Misses       1375     1368       -7     
Impacted Files Coverage Δ
include/faabric/scheduler/Scheduler.h 21.42% <ø> (ø)
src/planner/Planner.cpp 92.42% <97.43%> (+2.10%) ⬆️
src/endpoint/FaabricEndpointHandler.cpp 95.45% <100.00%> (+4.73%) ⬆️
src/mpi/MpiWorld.cpp 80.70% <100.00%> (+0.02%) ⬆️
src/planner/PlannerClient.cpp 80.00% <100.00%> (+4.39%) ⬆️
src/planner/PlannerServer.cpp 84.84% <100.00%> (+5.25%) ⬆️
src/scheduler/Executor.cpp 88.18% <100.00%> (ø)
src/scheduler/FunctionCallClient.cpp 87.20% <100.00%> (+1.87%) ⬆️
src/scheduler/FunctionCallServer.cpp 77.58% <100.00%> (+3.07%) ⬆️
src/scheduler/Scheduler.cpp 91.29% <100.00%> (+0.71%) ⬆️
... and 1 more

... and 4 files with indirect coverage changes

@csegarragonz csegarragonz merged commit e29062d into main Jun 21, 2023
@csegarragonz csegarragonz deleted the get-set-msg-result branch June 21, 2023 11:44
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.

1 participant
0