-
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] Add Autoscaler stub methods to GcsClients. #46061
Conversation
00f8945
to
e668e4c
Compare
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
e668e4c
to
57ae290
Compare
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
}, \ | ||
timeout_ms); \ | ||
return promise.get_future().get(); \ | ||
#define VOID_GCS_RPC_CLIENT_METHOD_FULL(NAMESPACE, \ |
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.
Could you add some documentation?
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.
added
virtual Status RequestClusterResourceConstraint( | ||
int64_t timeout_ms, | ||
const std::vector<std::unordered_map<std::string, double>> &bundles, | ||
const std::vector<int64_t> &count_array); | ||
|
||
virtual Status GetClusterResourceState(int64_t timeout_ms, | ||
std::string &serialized_reply); | ||
|
||
virtual Status GetClusterStatus(int64_t timeout_ms, std::string &serialized_reply); | ||
|
||
virtual Status ReportAutoscalingState(int64_t timeout_ms, | ||
const std::string &serialized_state); | ||
|
||
virtual Status DrainNode(const std::string &node_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.
Could you please add documentation?
request, &reply, timeout_ms); | ||
} | ||
|
||
Status AutoscalerStateAccessor::DrainNode(const std::string &node_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.
(Same for other methods) There are quite some code duplication with PythonGcsClient::DrainNode()
. I know eventually we want to replace PythonGcsClient
. But in the interim, is it possible to extract common code and invoke it from both? Wondering if we could avoid code diverging in the two places.
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 guess we are deleting the PythonGcsClient ones really quick so it's not that beneficial to do common functions with 2 wrappers.
Signed-off-by: Ruiyang Wang <rywang014@gmail.com>
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.
lg
ray::rpc::autoscaler::AutoscalerStateService is on GCS, accessible via PythonGcsClient but not via GcsClient. This PR adds all methods and brings feature parity for the latter.
In order to add it to GcsRpcClient, we find 2 challenges:
status
field like all other replies.To support this, I changed
VOID_GCS_RPC_CLIENT_METHOD
to a more generic versionVOID_GCS_RPC_CLIENT_METHOD_FULL
that accepts a namespace and a booleanhandle_payload_status
. The boolean is passed toinvoke_async_method
in compile time. This allows us to only checkstatus
field if the bool is true, and don't check it for the new autoscaler methods.The GcsClient methods are only added with sync version and not async version, because the callers only need sync ones.