-
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
Push snapshot diffs before reset, and thread results after #126
Conversation
private: | ||
void applyDiffsToSnapshot( | ||
const std::string& snapshotKey, | ||
const flatbuffers::Vector<flatbuffers::Offset<SnapshotDiffChunk>>* diffs); |
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 was never implemented/ used
id, | ||
threadPoolIdx, | ||
executingTaskCount); | ||
oldTaskCount - 1); |
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 logging wasn't thread-safe by accessing executingTaskCount
directly.
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
m.set_snapshotkey(snapshotKey); | ||
} | ||
|
||
// Dispatch the message, we expect them all to be executed other hosts |
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.
small typo "executed on other hosts"
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.
Good spot, will fix in next PR
When a thread completes on a remote host, we need to do three things: reset the executor, send the snapshot diffs, send the thread's results. Previously sending snapshot diffs was bundled with sending thread results, and done after resetting the executor. However, this doesn't make sense as the resetting may wipe out any changes that should be captured in the diffs.
Therefore, we need to send the diffs, reset the executor, then send the thread results (note that the thread results must always be sent after resetting the executor to ensure it's ready to be reused). There is already a stand-alone snapshot diffs message, so this PR switches to using that and removes the snapshot diffs from the thread results message.
The downside here is that completing a thread on a remote host now requires sending two messages rather than one, but this is unavoidable.
Changes:
Executor
code to push snapshot diffs before callingreset