[core] Split object manager into small C++ targets #50885
Conversation
| hdrs = [ | ||
| "src/ray/object_manager/common.h", | ||
| ], | ||
| deps = [ |
There was a problem hiding this comment.
Please cleanup the dependency as well, I don't think we need all common deps.
|
|
||
| ray_cc_library( | ||
| name = "object_manager_common", | ||
| srcs = [ |
There was a problem hiding this comment.
nit:
srcs = ["src/ray/object_manager/common.cc"],
hdrs = ["src/ray/object_manager/common.h"],
| "src/ray/object_manager/chunk_object_reader.h", | ||
| ], | ||
| deps = [ | ||
| ":spilled_object_reader", |
There was a problem hiding this comment.
Please add logging dependency as well.
| "src/ray/object_manager/spilled_object_reader.h", | ||
| ], | ||
| deps = [ | ||
| ":object_reader", |
There was a problem hiding this comment.
More dependencies are expected, including absl optional, common pb, logging.
| "src/ray/object_manager/object_reader.h", | ||
| ], | ||
| deps = [ | ||
| ":grpc_common_lib", |
There was a problem hiding this comment.
I don't think all dependencies are required, please only include least necessary ones.
| "src/ray/object_manager/ownership_based_object_directory.h", | ||
| ], | ||
| deps = [ | ||
| ":gcs_client_lib", |
There was a problem hiding this comment.
A few more deps should be included (include what you use).
There was a problem hiding this comment.
I also see opportunities to forward declare.
- instrumented_io_context
- GcsClient
- SubscriberInterface
Could be forward declared, in terms of ROI, I would suggest try GcsClient
| ":object_manager_rpc", | ||
| ":plasma_store_server_lib", | ||
| "//src/ray/util:counter_map", |
There was a problem hiding this comment.
Same here, please add all required dependencies.
| ":gcs", | ||
| "//src/ray/common:id", | ||
| "//src/ray/common:ray_config", | ||
| ":stats_metric", |
There was a problem hiding this comment.
Same here.
Also a nit, dependency order should be
(1) local dependencies first (targets under the same folder)
(2) repo dependencies
(3) external dependencies
There was a problem hiding this comment.
A few dependencies missing here, like absl hash map and set
Why are these changes needed?
Related issue number
Closes #50882
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.