-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[Core]Pull off timers out of heartbeat in raylet #13963
Conversation
Tests all failing... |
Looks like something wrong with pull manager, revert that part first and test are passed. |
ping @rkooo567 |
src/ray/util/util.cc
Outdated
@@ -362,3 +362,18 @@ std::shared_ptr<std::unordered_map<std::string, std::string>> ParseURL(std::stri | |||
result->emplace(key_value_pair.first, key_value_pair.second); | |||
return result; | |||
} | |||
|
|||
void RunFnPeriodically(std::function<void()> fn, boost::posix_time::milliseconds period, |
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.
Maybe we can change this to a helper class (say PeriodicRunner
), so the callers can just pass in a function and a period, without having to be aware of the timers.
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.
great! I'll change that.
Still seems to have many test failures. |
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
/// \class PeriodicalRunner | ||
/// A periodical runner attached with an io_context. | ||
/// It can run functions with specified period. Each function is triggered by its timer. | ||
/// To run a function, call `RunFnPeriodically(fn, period_ms)`. |
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.
Document that when the PeriodicalRunner object is destructed, the registered functions will stop running.
src/ray/util/periodical_runner.h
Outdated
~PeriodicalRunner(); | ||
|
||
void RunFnPeriodically(std::function<void()> fn, | ||
boost::posix_time::milliseconds period); |
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.
nit: this function can accept a long
, so callers don't need to construct a posix_time before calling it.
e9241bd
to
e893a74
Compare
…-project#13963)" (ray-project#14319)" This reverts commit 80657e5.
Why are these changes needed?
I found raylet mixed up many period actions with heartbeat, which is ugly and not easy for us to pick up heartbeat reporting into a separate thread.
This PR do 3 things:
RunFnPeriodically
.RunFnPeriodically
to replace dummy methods that dispatch task in a loop.Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.