[core] [6/N] Fix shared pointer usage for gcs server

[dentiny]

Cleanup shared pointer and use unique pointer for clear memory ownership and less error prune.

Previous PRs:

• [core] Cleanup shared pointer for local task manager #49026
• [core] Cleanup shared pointer usage for plasma #48993
• [core] [easy] Cleanup shared pointer for runtime env client #48975
• [core] Fix gcs server using shared pointer #48888
• [core] [5/N] Cleanup shared pointer for gcs actor scheduler #49031

[rynewang]

LGTM. though I think if we move forward this way we need to do proper lifetime annotations, e.g.

1. GcsServer outlives everyone.
2. GcsTableStorage, all GcsManagers share a lifetime, that starts from a GcsServer DoStart to Stop

and so on.

[dentiny]

LGTM. though I think if we move forward this way we need to do proper lifetime annotations, e.g.

1. GcsServer outlives everyone.
2. GcsTableStorage, all GcsManagers share a lifetime, that starts from a GcsServer DoStart to Stop

and so on.

Could you please tell me what should I add? C++ attributes? Or documentations?
Not sure if I understand correctly, my take is unique pointer itself is a lifecycle annotation, which implicit denotes class object outlives data member and owns B's lifecyle.

[dayshah]

what should the decision process be behind keeping things as unique ptr vs. just the obj and deleted copy constructors for the class

[dentiny]

I prefer to use unique pointer for all dependent class, which potentially involves IO operations.
I think the best strategy is to have

• a base class, which defines the interface
• an implementation class, which is the real production code
• and a mock class/fake class for unit test purpose

In GCS, most of the data members have their own logic, should be wrapped in unique pointer.

[dentiny]

• GcsTableStorage, all GcsManagers share a lifetime, that starts from a GcsServer DoStart to Stop

Discussed offline, comments added, PTAL.

[jjyao]

Merge confclit.

[dentiny]

Merge confclit.

@jjyao Yeah this PR has been on hold for a while... conflict should be resolved now.

[jjyao]

I see reference and pointer are both used. What's the rule you are following?

[dentiny]

• Prefer to use reference as data member, since it guarantee non-null;
• Have to use pointer in some places due to our testing implementation.

[dentiny]

In our test case, quite a few places are implemented as

Class(/*member1=*/nullptr, /*member2=*/nullptr)

meanwhile mem1 and mem2 have their own dependency which I don't want to touch, so using nullptr in some places, but the ownership should be clear.