[Core]Save task spec in separate table #22650

WangTaoTheTonic · 2022-02-25T07:57:23Z

Why are these changes needed?

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.

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 · 2022-03-01T01:47:23Z

is it still WIP?

ericl · 2022-03-01T02:08:40Z

Hmm don't we need it for restarting failed actors?

WangTaoTheTonic · 2022-03-01T02:57:57Z

is it still WIP?

Some dashboard test cases failed, I'll try to fix them today.

WangTaoTheTonic · 2022-03-01T02:58:45Z

Hmm don't we need it for restarting failed actors?

We store task spec in separate table and get them when needed.

WangTaoTheTonic · 2022-03-02T06:01:51Z

The failed windows tests and rllib tests are unrelated.

WangTaoTheTonic · 2022-03-03T02:45:00Z

@rkooo567 Could you help to review bro?

WangTaoTheTonic · 2022-03-28T02:45:43Z

@rkooo567 Is it ok to go?

src/ray/protobuf/gcs.proto

raulchen · 2022-03-30T09:28:44Z

src/ray/gcs/gcs_server/gcs_actor_manager.cc

@@ -790,6 +793,7 @@ void GcsActorManager::DestroyActor(const ActorID &actor_id,
      [this, actor_id, actor_table_data](Status status) {
        RAY_CHECK_OK(gcs_publisher_->PublishActor(
            actor_id, *GenActorDataOnlyWithStates(*actor_table_data), nullptr));
+        RAY_CHECK_OK(gcs_table_storage_->ActorTaskSpecTable().Delete(actor_id, nullptr));


There is an edge case where the actor is marked dead, but this delete doesn't finish. This will result in data leak.
It's not a big deal, but we can check this case when loading data after GCS restarts.

rkooo567 · 2022-03-30T22:33:02Z

Sorry. I’ve been off (until this week). Please feel free to merge the pr! @raulchen

rkooo567 · 2022-04-04T07:31:28Z

I am back. @WangTaoTheTonic I saw you added some new commits after it is approved. Is it ready to be merged? Also, can you add unit tests if those new code addition was due to some edge cases?

WangTaoTheTonic · 2022-04-07T01:57:58Z

I am back. @WangTaoTheTonic I saw you added some new commits after it is approved. Is it ready to be merged? Also, can you add unit tests if those new code addition was due to some edge cases?

Yeah, it's ready to be merged.
I added some codes just to prevent the situation that the gcs crashes (no matter what reason) before the task spec is removing but the actor table item has been removed, per @raulchen comments. It's a small sanity check and won't affect the whole pr too much.
The unit test is hard to simulate that situation as we must kill gcs right in the duration between actor table item deleting and task spec item deleting.

WangTaoTheTonic · 2022-04-07T02:01:39Z

And the failed test cases are not related except the LinkCheck, @rkooo567 could you help to confirm?

WangTaoTheTonic · 2022-04-08T03:29:02Z

The failed reason of the two cases are same. It looks like the dashboard frontend issue and not related.

WangTaoTheTonic · 2022-04-08T03:47:30Z

@edoakes Hi Edward, this patch changes some proto structures in actor data table, so it impacts dashboard too. The change is only about messages but not api change.
Could you help to take a review? As we need approvals of owners from modules involved.

edoakes · 2022-04-08T15:56:41Z

dashboard/modules/actor/actor_head.py

        "timestamp",
        "numExecutedTasks",
    }
    light_message = {k: v for (k, v) in orig_message.items() if k in fields}
-    if "taskSpec" in light_message:
-        actor_class = actor_classname_from_task_spec(light_message["taskSpec"])


please delete actor_classname_from_task_spec if it's no longer used anywhere

We just don't use it here. It's still used by other codes.

edoakes · 2022-04-08T15:58:24Z

dashboard/modules/actor/actor_head.py

-        if "functionDescriptor" in light_message["taskSpec"]:
-            light_message["taskSpec"] = {
-                "functionDescriptor": light_message["taskSpec"]["functionDescriptor"]
-            }


this looks like we're changing the returned schema -- we used to return light_message["taskSpec"]["functionDescriptor"] but now it's just light_message["functionDescriptor"]. We likely need to change the dashboard to account for this?

The dashboard uses function descriptor just for showing, we extract it from the returned schema, no matter in what position it is.

rkooo567 · 2022-04-11T13:26:17Z

Btw, I need the approval from the protobuf code owner cc @edoakes @wuisawesome

wuisawesome

the code owner parts lgtm, didn't review the rest

rkooo567 · 2022-04-12T19:24:13Z

Failed test must be unrelated

simon-mo · 2022-04-26T00:23:34Z

src/ray/gcs/gcs_server/gcs_actor_manager.cc

@@ -372,6 +375,8 @@ void GcsActorManager::HandleGetNamedActorInfo(
  } else {
    reply->unsafe_arena_set_allocated_actor_table_data(
        iter->second->GetMutableActorTableData());
+    RAY_LOG(INFO) << "WANGTAO " << iter->second->GetState();


please remove this log line or make this more informational

@simon-mo Sorry for missing that, will remove it in #24204

WangTaoTheTonic added 2 commits February 24, 2022 10:23

add TaskSpec table

9ea1e38

[Core]Do not save task spec in Gcs actor table

e246d13

WangTaoTheTonic requested review from raulchen, kfstorm and rkooo567 February 25, 2022 07:57

WangTaoTheTonic requested review from wuisawesome, ericl, AmeerHajAli, robertnishihara and pcmoritz as code owners February 25, 2022 07:57

WangTaoTheTonic requested a review from ffbin February 25, 2022 08:06

rkooo567 assigned rkooo567 and scv119 Feb 26, 2022

lint

a2f5b9f

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

WangTaoTheTonic added 2 commits March 1, 2022 19:53

fix dashboard tests

e9c7c5b

reformatted

3051491

WangTaoTheTonic requested review from ijrsvt, edoakes, alanwguo and architkulkarni as code owners March 1, 2022 11:57

WangTaoTheTonic changed the title ~~[WIP][Core]Do not save task spec in Gcs actor table~~ [Core]Save task spec in separate table Mar 1, 2022

WangTaoTheTonic removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 1, 2022

lint

ed5bc4d

raulchen reviewed Mar 30, 2022

View reviewed changes

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

WangTaoTheTonic added 3 commits March 30, 2022 14:38

temp

fc52660

remove some fields

56a5da1

lint

97d0dc2

raulchen reviewed Mar 30, 2022

View reviewed changes

raulchen approved these changes Mar 30, 2022

View reviewed changes

WangTaoTheTonic added 9 commits March 31, 2022 13:46

clean task spec of actor when initializing

de682e5

Merge branch 'master' into task_spec

31cfc94

add if

8424f89

Merge branch 'master' into task_spec

2fb1d3d

typo

c38665b

hotfix

c190e1e

Merge branch 'master' into task_spec

53466da

fix tests

583a9ea

lint

234f909

edoakes reviewed Apr 8, 2022

View reviewed changes

edoakes approved these changes Apr 11, 2022

View reviewed changes

wuisawesome approved these changes Apr 12, 2022

View reviewed changes

rkooo567 merged commit 6aefe9b into ray-project:master Apr 12, 2022

WangTaoTheTonic deleted the task_spec branch April 13, 2022 06:46

simon-mo reviewed Apr 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core]Save task spec in separate table #22650

[Core]Save task spec in separate table #22650

WangTaoTheTonic commented Feb 25, 2022 •

edited

Loading

rkooo567 commented Mar 1, 2022

ericl commented Mar 1, 2022

WangTaoTheTonic commented Mar 1, 2022

WangTaoTheTonic commented Mar 1, 2022 •

edited

Loading

WangTaoTheTonic commented Mar 2, 2022

WangTaoTheTonic commented Mar 3, 2022

WangTaoTheTonic commented Mar 28, 2022

raulchen Mar 30, 2022

rkooo567 commented Mar 30, 2022

rkooo567 commented Apr 4, 2022

WangTaoTheTonic commented Apr 7, 2022

WangTaoTheTonic commented Apr 7, 2022

WangTaoTheTonic commented Apr 8, 2022

WangTaoTheTonic commented Apr 8, 2022

edoakes Apr 8, 2022

WangTaoTheTonic Apr 11, 2022

edoakes Apr 8, 2022

WangTaoTheTonic Apr 11, 2022

rkooo567 commented Apr 11, 2022

wuisawesome left a comment

rkooo567 commented Apr 12, 2022

simon-mo Apr 26, 2022

WangTaoTheTonic Apr 26, 2022

[Core]Save task spec in separate table #22650

[Core]Save task spec in separate table #22650

Conversation

WangTaoTheTonic commented Feb 25, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 commented Mar 1, 2022

ericl commented Mar 1, 2022

WangTaoTheTonic commented Mar 1, 2022

WangTaoTheTonic commented Mar 1, 2022 • edited Loading

WangTaoTheTonic commented Mar 2, 2022

WangTaoTheTonic commented Mar 3, 2022

WangTaoTheTonic commented Mar 28, 2022

raulchen Mar 30, 2022

Choose a reason for hiding this comment

rkooo567 commented Mar 30, 2022

rkooo567 commented Apr 4, 2022

WangTaoTheTonic commented Apr 7, 2022

WangTaoTheTonic commented Apr 7, 2022

WangTaoTheTonic commented Apr 8, 2022

WangTaoTheTonic commented Apr 8, 2022

edoakes Apr 8, 2022

Choose a reason for hiding this comment

WangTaoTheTonic Apr 11, 2022

Choose a reason for hiding this comment

edoakes Apr 8, 2022

Choose a reason for hiding this comment

WangTaoTheTonic Apr 11, 2022

Choose a reason for hiding this comment

rkooo567 commented Apr 11, 2022

wuisawesome left a comment

Choose a reason for hiding this comment

rkooo567 commented Apr 12, 2022

simon-mo Apr 26, 2022

Choose a reason for hiding this comment

WangTaoTheTonic Apr 26, 2022

Choose a reason for hiding this comment

WangTaoTheTonic commented Feb 25, 2022 •

edited

Loading

WangTaoTheTonic commented Mar 1, 2022 •

edited

Loading