[core] Refactor ActorInfoAccessor in gcs_client so as to be mockable

[ZacAttack]

Why are these changes needed?

The gcs_client behavior today is hard to overload/override. This is bad for a few reasons. One being that it can be difficult to instrument testing. There are examples today amongst accessors that need to give ‘Friend’ level access to methods to different classes in order to accomplish some of the tests today.

To resolve this, we're going to introduce some new abstractions. This is the first PR that sets up some of the framework. From here, we'll do the remainder of the accessors.

New Classes

actor_info_accessor.cc <-- Concretion implementations 
actor_info_accessor.h  <-- Concretion declarations (contains private methods and members)
actor_info_accessor_interface.h <-- Interface function declarations

gcs_client and all other accessor users will interact now with an ActorInfoAccessorInterface object. Next in order to make this injectable, we need to pull away the inline concretion instantiation going on in gcs_client.connect(). To do this, we are going to create a new factory interface called AccessorFactoryInterface which is included in this PR.

There is one other class/abstraction we need to introduce. Ideally, in order to make this pluggable, we need separated build targets between interfaces and implementations and gcs_client. Before, everything was bundled together. But since we’re moving things out, we need to break the circular dependency between different accessors and the gcs_client. The main reason why the gcs_client passes itself into each accessor is that today these different accessors try to share a subscriber and a rpc_client (and potentially more in the future). So we introduce a new interface to break this cycle. It is the GcsClientContext. The intention of this object is to be a ‘grab bag’ of objects that are needed by accessor implementations.

Related issue number

#54805

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run pre-commit jobs to lint the changes in this PR. (pre-commit setup)
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[gemini-code-assist]

Code Review

This pull request is a well-executed refactoring of ActorInfoAccessor to make it mockable, which is a great improvement for testability. The introduction of ActorInfoAccessorInterface, AccessorFactoryInterface, and GcsClientContext provides a clean separation of concerns and enables dependency injection. The changes are consistently applied, and the new unit test for the injection mechanism is a valuable addition. I have a couple of minor suggestions to further improve the code quality.

[codope]

Overall, a sound refactoring and totally aligned with the approach. I wonder if we can document the best practices in a markdown (hard to enforce through linter) and people can copy it in their claude.md (or whatever coding assistant if being used)! :)

GcsClientContext. The intention of this object is to be a ‘grab bag’ of objects that are needed by accessor implementations.

Pragmatic but could become a dumping ground. Please consider documenting what should and shouldn't go in this context. Like shared resources (RpcClient, Subscriber), common configs can go but not state specific to one accessor.

[edoakes]

Looks like a great improvement. The main issue I see with this is that all of the accessors still depend on a shared gRPC client, which means if you add/update/remove any of the RPC services to the GCS, it will still require recompiling all accessor implementations and therefore all downstream components :(

In an ideal world we'd be actually separating things out along the same lines that are drawn in the protos. That is, for each logical service define a gRPC service. Then each one has an isolated gRPC client implementation and there is no centralized implementation file that requires rebuilding the world. They can still share the same completion queues/threads.

Something like:

# proto file
service ActorInfoService {
...
}

# cpp files
actor_info_accessor_interface.h
actor_info_accessor.h

# actor info accessor encapsulates the gRPC client logic for its service
actor_info_accessor_(shared_completion_queue)

[github-actions]

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[israbbani]

LGTM. Minor nits.

I think for everything that's base class, you need to use a virtual destructor.

[israbbani]

Followup? Remove the outparam and use StatusSetOr or whatever.

[israbbani]

🚢