[core] Split out periodical_runner_interface.h build target#63205
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors PeriodicalRunnerInterface into a separate header and library target to decouple the interface from its implementation. Corresponding updates were made to build files and headers across the repository to reference the new interface target. The review feedback suggests including <cstdint> in the new header to ensure it is self-contained when using uint64_t.
| #include <functional> | ||
| #include <memory> | ||
| #include <string> |
There was a problem hiding this comment.
The header uses uint64_t in the RunFnPeriodically method but does not explicitly include <cstdint>. It is best practice to include this header to ensure the file is self-contained and to avoid potential compilation issues in different environments.
| #include <functional> | |
| #include <memory> | |
| #include <string> | |
| #include <cstdint> | |
| #include <functional> | |
| #include <memory> | |
| #include <string> |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 80f5e70. Configure here.
| @@ -0,0 +1,48 @@ | |||
| // Copyright 2017 The Ray Authors. | |||
There was a problem hiding this comment.
super nit: we aren't in 2017 anymore
| std::string name) = 0; | ||
|
|
||
| protected: | ||
| virtual void DoRunFnPeriodically( |
There was a problem hiding this comment.
not related: first time I'm seeing pure virtual + in protected which is a bit odd... looks like these are just used in periodical_runner.cc as helper functions so prob better to just move them as private helpers in periodical_runner.h and out of the interface class.
2 participants
Follow-up from: #63042