[GCS]Do not save task spec in GCS actor table(Part1) #11592

ffbin · 2020-10-24T08:50:01Z

Why are these changes needed?

In the 200 node cluster test, we found that after GCS restart, it takes a long time for GCS client to fetch actor information from GCS server, because the task spec is large and the serialization time is long. And GCS will mistakenly think that the node heartbeat timeout.
Currently, task spec is only required when GCS creates or restarts an actor. Publishing an actor and getting an actor/actors do not need to return a task spec, so we store task spec in actor task spec table separately.
This feature is split into three PR:
Currently, it is the first PR, and the second PR supports synchronous acquisition of actor task spec data. The third PR deals with the recovery process after GCS restart.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
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 :(

rkooo567 · 2020-11-11T02:24:33Z

Is it ready for review? Can you resolve the merge conflict and remove the tag in that case?

ffbin · 2020-11-11T02:59:28Z

Is it ready for review? Can you resolve the merge conflict and remove the tag in that case?

Thanks, i will resolve the merge conflict. I find that we can't remove task_spec directly because the new dashboard depends on it.

rkooo567 · 2020-11-11T03:01:15Z

What are values that the dashboard depends on? We can probably extract them when creating an Actor class in gcs_actor_manager.cc. I am sure only a few fields are used for the dashboard, and it is probably waste to keep it just for the dashboard.

ffbin · 2020-11-11T12:28:40Z

What are values that the dashboard depends on? We can probably extract them when creating an Actor class in gcs_actor_manager.cc. I am sure only a few fields are used for the dashboard, and it is probably waste to keep it just for the dashboard.

Does removing task_spec affect get_actor_creation_tasks function?

rkooo567 · 2020-11-11T16:53:48Z

@ffbin

def actor_classname_from_task_spec(task_spec):
    return task_spec.get("functionDescriptor", {})\
                .get("pythonFunctionDescriptor", {})\
                .get("className", "Unknown actor class")

We only use these 3 fields, so keeping the whole task spec would be wasteful?

Maybe we can create a "metadata" field which contains data that is required by the dashboard (for now just actor class name).

rkooo567 · 2020-11-11T16:56:19Z

But cc @mfitton just in case

mfitton · 2020-11-11T22:14:33Z

@rkooo567 @ffbin Sang is right that we only need those fields from the task spec, but we pull that data from the GetNodeStatsReply protobuf (defined in node_manager.proto), so some kind of task spec data (even if it has unnecessary fields removed) will need to remain in that reply. This data is currently supplied in the NodeManager function HandleGetNodeStats taken from the local_queues_ attribute.

If this data is removed from the reply, we will no longer be able to show users information about pending and infeasible actors within the logical view.

Correct me if I'm wrong because I only glanced through the PR, but it doesn't seem like this affects the data stored in the local task queue per node at all, so it shouldn't actually affect the dashboard.

ffbin · 2020-11-12T02:21:09Z

@rkooo567 @ffbin Sang is right that we only need those fields from the task spec, but we pull that data from the GetNodeStatsReply protobuf (defined in node_manager.proto), so some kind of task spec data (even if it has unnecessary fields removed) will need to remain in that reply. This data is currently supplied in the NodeManager function HandleGetNodeStats taken from the local_queues_ attribute.

If this data is removed from the reply, we will no longer be able to show users information about pending and infeasible actors within the logical view.

Correct me if I'm wrong because I only glanced through the PR, but it doesn't seem like this affects the data stored in the local task queue per node at all, so it shouldn't actually affect the dashboard.

Thank you for your detailed explains :)

rkooo567

LGTM in general. Just a couple comments.

Btw, does it mean after this PR is merged actor restart or gcs restart will still work? Or should we wait until 3 PRs are merged?

src/ray/protobuf/gcs.proto

src/ray/gcs/gcs_server/gcs_actor_manager.cc

ffbin · 2020-11-12T09:34:29Z

LGTM in general. Just a couple comments.

Btw, does it mean after this PR is merged actor restart or gcs restart will still work? Or should we wait until 3 PRs are merged?

After this PR is merged, gcs restart will not work until 3 PRs are merged, thanks.

rkooo567 · 2020-11-13T01:13:46Z

@ffbin Can you add skip for gcs server restart tests?

ffbin · 2020-11-13T01:40:51Z

@ffbin Can you add skip for gcs server restart tests?

Oh, i will add skip for gcs server restart tests, thanks.

ffbin · 2020-12-03T12:33:52Z

Hi @rkooo567 , Can you help review this PR, thanks :)

rkooo567 · 2020-12-04T08:15:48Z

python/ray/tests/test_gcs_fault_tolerance.py

@@ -47,6 +47,7 @@ def test_gcs_server_restart(ray_start_regular):
    assert result == 2


+@pytest.mark.skip("This test does not work yet.")


Is it true?

Oops nvm. I re-read the description.

src/ray/gcs/gcs_server/gcs_actor_manager.cc

src/ray/protobuf/gcs.proto

rkooo567 · 2021-01-05T22:28:26Z

cc @mfitton

rkooo567 · 2021-02-22T18:34:18Z

Hey @ffbin do you have a plan to merge this soon?

ffbin · 2021-02-23T02:34:01Z

Hey @ffbin do you have a plan to merge this soon?

Yeah, this will bring great benefits to the performance. I will see how to solve the problem of dashboard.

rkooo567 · 2021-06-16T09:25:09Z

cc @ffbin any status on this PR?

ffbin · 2021-06-17T02:47:04Z

@rkooo567 Sorry, I haven't invested in this PR recently. I will ask @clay4444 to help me finish it.

rkooo567 · 2021-11-23T12:03:21Z

cc @ffbin no plan to work on this further? Should I close it?

kfstorm · 2021-11-26T04:15:02Z

@clay4444 Could you update the status?

bveeramani · 2022-01-30T06:01:57Z

‼️ ACTION REQUIRED ‼️

We've switched our code formatter from YAPF to Black (see #21311).

To prevent issues with merging your code, here's what you'll need to do:

Install Black

pip install -I black==21.12b0

Format changed files with Black

curl -o format-changed.sh https://gist.githubusercontent.com/bveeramani/42ef0e9e387b755a8a735b084af976f2/raw/7631276790765d555c423b8db2b679fd957b984a/format-changed.sh
chmod +x ./format-changed.sh
./format-changed.sh
rm format-changed.sh

Commit your changes.

git add --all
git commit -m "Format Python code with Black"

Merge master into your branch.

git pull upstream master

Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated format.sh.

WangTaoTheTonic · 2022-02-10T03:44:38Z

I'll pick this up.

stale · 2022-03-12T21:28:24Z

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

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

kfstorm · 2022-03-13T09:22:52Z

‼️ ACTION REQUIRED ‼️

We've updated our formatting configuration for C++ code. (see #22725)

This PR includes C++ code change. To prevent issues with merging your code, here's what you'll need to do:

Merge the latest changes from upstream/master branch into your branch.

git pull upstream master
git merge upstream/master

Resolve merge conflicts (if necessary).

After running these steps, you'll have the updated C++ formatting configuration.

Format changed files.

scripts/format.sh

Commit your changes.

git add --all
git commit -m "Format C++ code"

This is a rebase version of #11592. As task spec info is only needed when gcs create or start an actor, so we can remove it from actor table and save the serialization time and memory/network cost when gcs clients get actor infos from gcs. As internal repository varies very much from the community. This pr just add some manual check with simple cherry pick. Welcome to comment first and at the meantime I'll see if there's any test case failed or some points were missed.

ffbin requested a review from rkooo567 October 24, 2020 09:02

ffbin assigned rkooo567 Oct 24, 2020

ffbin added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 25, 2020

ffbin changed the title ~~[GCS]Add actor task_spec table(Part1)~~ [GCS]Separate actor task_spec from actor table(Part1) Oct 25, 2020

ffbin force-pushed the add_actor_task_spec_table branch from 18da7de to b625dd6 Compare November 11, 2020 09:34

ffbin changed the title ~~[GCS]Separate actor task_spec from actor table(Part1)~~ [GCS]Do not save task spec in GCS actor table(Part1) Nov 11, 2020

ffbin removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 11, 2020

rkooo567 reviewed Nov 12, 2020

View reviewed changes

src/ray/protobuf/gcs.proto Outdated Show resolved Hide resolved

src/ray/protobuf/gcs.proto Show resolved Hide resolved

src/ray/gcs/gcs_server/gcs_actor_manager.cc Show resolved Hide resolved

src/ray/gcs/gcs_server/gcs_actor_manager.cc Outdated Show resolved Hide resolved

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Nov 13, 2020

ffbin force-pushed the add_actor_task_spec_table branch from c0a39b7 to dd89730 Compare November 28, 2020 08:30

ffbin requested a review from rkooo567 November 30, 2020 09:23

ffbin force-pushed the add_actor_task_spec_table branch 2 times, most recently from de2dd35 to 59000a6 Compare December 2, 2020 08:05

ffbin removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 2, 2020

rkooo567 reviewed Dec 4, 2020

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Dec 4, 2020

ffbin force-pushed the add_actor_task_spec_table branch from 03541e8 to 78e1abe Compare December 4, 2020 09:00

ffbin requested a review from rkooo567 December 5, 2020 07:09

ffbin force-pushed the add_actor_task_spec_table branch from 78e1abe to 46216bb Compare February 23, 2021 06:56

灵洵 and others added 8 commits April 6, 2021 17:47

add part code

73f4d2d

fix proto bug

722f258

add part code

89cf96c

fix compile bug

5775b18

fix review comment

3387b0b

fix bug

cf645ed

fix ut bug

8bcca58

fix review comments

508c334

ffbin force-pushed the add_actor_task_spec_table branch from 46216bb to 508c334 Compare April 6, 2021 09:53

fix compile bug

635eced

ffbin force-pushed the add_actor_task_spec_table branch from bc6db6e to 635eced Compare April 8, 2021 01:54

灵洵 added 2 commits April 8, 2021 10:06

Get actor class name from function descriptor

21dcac3

fix lint error

994d434

WangTaoTheTonic mentioned this pull request Feb 25, 2022

[Core]Save task spec in separate table #22650

Merged

6 tasks

stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 12, 2022

stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Mar 13, 2022

ffbin closed this Apr 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCS]Do not save task spec in GCS actor table(Part1) #11592

[GCS]Do not save task spec in GCS actor table(Part1) #11592

ffbin commented Oct 24, 2020 •

edited

Loading

rkooo567 commented Nov 11, 2020

ffbin commented Nov 11, 2020

rkooo567 commented Nov 11, 2020

ffbin commented Nov 11, 2020

rkooo567 commented Nov 11, 2020 •

edited

Loading

rkooo567 commented Nov 11, 2020

mfitton commented Nov 11, 2020 •

edited

Loading

ffbin commented Nov 12, 2020

rkooo567 left a comment

ffbin commented Nov 12, 2020

rkooo567 commented Nov 13, 2020

ffbin commented Nov 13, 2020

ffbin commented Dec 3, 2020

rkooo567 Dec 4, 2020

rkooo567 Dec 4, 2020

rkooo567 commented Jan 5, 2021

rkooo567 commented Feb 22, 2021

ffbin commented Feb 23, 2021

rkooo567 commented Jun 16, 2021

ffbin commented Jun 17, 2021

rkooo567 commented Nov 23, 2021

kfstorm commented Nov 26, 2021

bveeramani commented Jan 30, 2022

WangTaoTheTonic commented Feb 10, 2022

stale bot commented Mar 12, 2022

kfstorm commented Mar 13, 2022

		@@ -47,6 +47,7 @@ def test_gcs_server_restart(ray_start_regular):
		assert result == 2


		@pytest.mark.skip("This test does not work yet.")

[GCS]Do not save task spec in GCS actor table(Part1) #11592

[GCS]Do not save task spec in GCS actor table(Part1) #11592

Conversation

ffbin commented Oct 24, 2020 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 commented Nov 11, 2020

ffbin commented Nov 11, 2020

rkooo567 commented Nov 11, 2020

ffbin commented Nov 11, 2020

rkooo567 commented Nov 11, 2020 • edited Loading

rkooo567 commented Nov 11, 2020

mfitton commented Nov 11, 2020 • edited Loading

ffbin commented Nov 12, 2020

rkooo567 left a comment

Choose a reason for hiding this comment

ffbin commented Nov 12, 2020

rkooo567 commented Nov 13, 2020

ffbin commented Nov 13, 2020

ffbin commented Dec 3, 2020

rkooo567 Dec 4, 2020

Choose a reason for hiding this comment

rkooo567 Dec 4, 2020

Choose a reason for hiding this comment

rkooo567 commented Jan 5, 2021

rkooo567 commented Feb 22, 2021

ffbin commented Feb 23, 2021

rkooo567 commented Jun 16, 2021

ffbin commented Jun 17, 2021

rkooo567 commented Nov 23, 2021

kfstorm commented Nov 26, 2021

bveeramani commented Jan 30, 2022

‼️ ACTION REQUIRED ‼️

WangTaoTheTonic commented Feb 10, 2022

stale bot commented Mar 12, 2022

kfstorm commented Mar 13, 2022

‼️ ACTION REQUIRED ‼️

ffbin commented Oct 24, 2020 •

edited

Loading

rkooo567 commented Nov 11, 2020 •

edited

Loading

mfitton commented Nov 11, 2020 •

edited

Loading