-
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
Retry GET
from Redis for messages in the Execution Graph
#181
Conversation
b56176e
to
6a0754a
Compare
REQUIRE(node.msg.id() == req->mutable_messages()->at(i).id()); | ||
} | ||
} | ||
} |
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.
This test isn't checking that it will fail when the max retries are hit, which seems like the easier case to check for.
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 add another test that checks that you can't get an execution graph for a message that hasn't been published.
…for it in the tests
348b483
to
11194b2
Compare
In this PR I fix a race condition that happened when running
Scheduler::getFunctionExecGraph()
immediately afterScheduler::getFunctionResult()
in a distributed setting.In particular, we sometimes generated the execution graph before all remote executors had published their resulting message. As a consequence,
redis.get
would return an empty byte array, and we'd cast it to an emptyfaabric::Message
.I add a naive retry mechanism (bear in mind generating execution graphs is off the hot path), and a test that reproduces the failure with very little retries (in fact 10 tries might be an overkill).