[core] Fix gcs server using shared pointer #48888
Conversation
Signed-off-by: hjiang <[email protected]>
| /// The gcs resource manager. | ||
| std::shared_ptr<GcsResourceManager> gcs_resource_manager_; | ||
| std::unique_ptr<GcsResourceManager> gcs_resource_manager_; | ||
| /// The cluster resource scheduler. | ||
| std::shared_ptr<ClusterResourceScheduler> cluster_resource_scheduler_; |
There was a problem hiding this comment.
There're still a few shared pointer usage, which I find hard to adapt, leave it to next cleanup.
Signed-off-by: hjiang <[email protected]>
| @@ -248,7 +248,7 @@ class GcsNodeManager : public rpc::NodeInfoHandler { | |||
| /// Storage for GCS tables. | |||
| std::shared_ptr<gcs::GcsTableStorage> gcs_table_storage_; | |||
| /// Raylet client pool. | |||
| std::shared_ptr<rpc::NodeManagerClientPool> raylet_client_pool_; | |||
| rpc::NodeManagerClientPool *raylet_client_pool_ = nullptr; | |||
There was a problem hiding this comment.
This is never nullptr, so use&, not * ?
There was a problem hiding this comment.
afaik most style guides suggest pointers over references because of flexibility, moving, copying, it doesn't apply to this, but not bad to be consistent, but can we do absl::NotNull, I don't remember if we need cpp upgrade or absl upgrade for it?
There was a problem hiding this comment.
+1 for using reference if it's guaranteed to be not null.
There was a problem hiding this comment.
IMHO our use case here does not require flexibility or so: we won't reassign, copy or move them. I'd say we like the fact that it can't (easiliy) be moved by using reference. The only caution is that we need to make sure the references must outlive the objects holding those refs, which is the case since all these classes share a lifetime with a GcsServer (up until a reset call).
There was a problem hiding this comment.
Because we have use cases passing nullptr:
ray/src/ray/gcs/gcs_server/test/gcs_actor_scheduler_mock_test.cc
Lines 41 to 42 in ba8674a
| @@ -181,15 +183,15 @@ rpc::PlacementGroupStats *GcsPlacementGroup::GetMutableStats() { | |||
|
|
|||
| GcsPlacementGroupManager::GcsPlacementGroupManager( | |||
| instrumented_io_context &io_context, | |||
| std::shared_ptr<GcsPlacementGroupSchedulerInterface> scheduler, | |||
| GcsPlacementGroupSchedulerInterface *scheduler, | |||
There was a problem hiding this comment.
This is never nullptr, so use&, not * ?
There was a problem hiding this comment.
Same here, we have places passing in nullptr.
|
@rynewang / @dayshah / @jjyao My take / preference on reference vs pointer:
@dayshah On coding practice,
|
Signed-off-by: hjiang <[email protected]>
Current implementation for shared pointer usage is completely a mess;
Shared pointer should ONLY be used where lifecycle is impossible to judge, for example, async operations or multi-threaded cases; abusing would lead to memory leak (i.e. circular dependency), and makes code hard to justify.