-
Notifications
You must be signed in to change notification settings - Fork 15
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
Change flush to execute once per host #112
Conversation
@@ -66,9 +66,6 @@ void FunctionCallServer::recvFlush(faabric::transport::Message& body) | |||
|
|||
// Clear the scheduler | |||
scheduler.flushLocally(); | |||
|
|||
// Reset the scheduler | |||
scheduler.reset(); |
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.
Scheduler reset
gets called in flushLocally
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.
LGTM. However, the dummy tests look indeed a bit dumb. Maybe not in this PR, but is there a way we could actually test the issue reported in faasm/faasm#381 ? Maybe in the distributed tests?
I've also ran into the same issue myself after the issue was closed.
All the complexity around flushing is in Faasm so that's where the tests should be. It's a good point re. that issue and it's something that's been hanging around for a while. A more rigorous distributed test in Faasm would be good. |
This reverts commit 253c4c6.
Flushing is a one-off host-wide task that needs to be called after all the executors have terminated. However, flushing was previously implemented as an instance method on the
Executor
, meaning it was unnecessarily called repeatedly and wouldn't actually get called if there were no executors present. Therefore I've moved it to a method on theExecutorFactory
(which is a singleton), so it now gets called once per host.