Add a flag to disable reconstruction for a killed actor by kfstorm · Pull Request #7346 · ray-project/ray

kfstorm · 2020-02-27T07:38:23Z

Add local_raylet_client_->Disconnect() in CoreWorker::HandleKillActor to mark the actor as dead instead of reconstructing, if the no_reconstruction flag is set to true.

Currently the flag is exposed in Java only.

AmplabJenkins · 2020-02-27T07:42:06Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-02-27T08:32:44Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22489/
Test FAILed.

AmplabJenkins · 2020-02-27T11:18:16Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22492/
Test FAILed.

edoakes · 2020-02-27T22:20:15Z

@kfstorm I'm not sure about this change actually. KillActor is meant for situations in which the actor is hanging and the user might want to kill a hanging actor and have it reconstructed to be usable again. If you want the actor not to be reconstructed, it should go through the normal actor termination path (in python, sending actor.__ray_terminate__.remote()

jovany-wang · 2020-02-28T02:48:00Z

ps:Not related to this PR.

I'don't know if it's too late to suggest renaming it to exitActorGracefully or some words which can indicates the API is used for killing actor with not reconstructing. I guess it would be more obvious.

edoakes · 2020-02-28T17:53:59Z

@kfstorm based on the discussion on #7360, could you make this optional? That is, add a flag (that's off by default) that calls local_raylet_client_->Disconnect().

mitar · 2020-02-29T09:13:31Z

I think the best would be to have API:

ray.kill_actor (calls current kill + disconnect so that it is not restarted)
ray.restart_actor (calls current kill)
ray.terminate_actor (calls __ray_terminate__)

kfstorm · 2020-03-01T05:28:15Z

@edoakes Sounds good. I'll update this PR. @mitar What's the semantics of invoking ray.restart_actor against a non-reconstructable actor?

mitar · 2020-03-01T05:57:25Z

What's the semantics of invoking ray.restart_actor against a non-reconstructable actor?

It could be that it re-creates with initial state? Instead of reconstructed state? (I am assuming what reconstructable actor is, I am not really familiar with that.)

edoakes · 2020-03-01T19:43:34Z

@kfstorm I don't think we want to expose this in the python API without more discussion about the API/semantics, so maybe add the flag but don't make it public yet?

AmplabJenkins · 2020-03-04T09:00:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22717/
Test PASSed.

AmplabJenkins · 2020-03-04T16:30:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22734/
Test PASSed.

edoakes · 2020-03-04T21:32:00Z

@kfstorm looks good, but see my comment on #7434.

raulchen · 2020-03-05T04:06:49Z

java/api/src/main/java/org/ray/api/Ray.java

   */
-  public static void killActor(RayActor<?> actor) {
-    runtime.killActor(actor);
+  public static void killActor(RayActor<?> actor, boolean noReconstruction) {


Also move this API to RayActor?

raulchen · 2020-03-05T04:07:38Z

java/test/src/main/java/org/ray/api/test/KillActorTest.java

+  public static class KillerActor {
+
+    public void kill(RayActor<?> actor, boolean noReconstruction) {
+      Ray.killActor(actor, noReconstruction);


I've merged the new API, can you rebase and update this?

raulchen · 2020-03-05T04:10:49Z

python/ray/worker.py


    worker = ray.worker.get_global_worker()
-    worker.core_worker.kill_actor(actor._ray_actor_id)
+    worker.core_worker.kill_actor(actor._ray_actor_id, False)


Should we expose an option to this kill method? It can default to False.

@edoakes just saw your comment above. I think it makes more sense to add an option here and deprecate __ray_terminate__.

Because, 1) having two different APIs (kill and __ray_terminate__) is confusing to users. 2) __ray_terminate__ is also an actor method, if the actor is hanging, it won't take effect.

I agree that having ray.kill and actor.__ray_terminate__ is confusing. They have similar effects but with different approaches. We can combine them into one API.

Completely agree - ray.kill with a "force kill" flag seems best to me

AmplabJenkins · 2020-03-06T02:56:15Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22794/
Test FAILed.

kfstorm · 2020-03-11T14:20:36Z

src/ray/core_worker/core_worker.cc

-            RAY_CHECK_OK(KillActor(actor_id, /*intentional=*/true));
+            RAY_CHECK_OK(
+                KillActor(actor_id, /*force_kill=*/true, /*no_reconstruction=*/false));


@stephanie-wang Maybe the second argument should be false? Do you try to destroy the actor gracefully?

AmplabJenkins · 2020-03-11T15:04:56Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23034/
Test FAILed.

AmplabJenkins · 2020-03-12T04:16:32Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23061/
Test FAILed.

AmplabJenkins · 2020-03-12T12:01:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23090/
Test FAILed.

AmplabJenkins · 2020-03-12T12:29:35Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23092/
Test FAILed.

raulchen · 2020-03-13T06:45:46Z

Can you merge master and resolve the conflicts?

AmplabJenkins · 2020-03-13T08:26:21Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/23138/
Test PASSed.

A killed actor should not be reconstructed.

73c396e

kfstorm requested review from edoakes, jovany-wang, raulchen and zhijunfu February 27, 2020 07:38

fix

2d08be6

kfstorm changed the title ~~[Java] A killed actor should not be reconstructed.~~ A killed actor should not be reconstructed. Feb 27, 2020

robertnishihara mentioned this pull request Feb 28, 2020

Change actor.__ray_kill__() to ray.kill(actor) #7360

Merged

3 tasks

kfstorm added 2 commits March 4, 2020 13:01

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

0f3326d

Add a flag to disable reconstruction

d09da91

kfstorm changed the title ~~A killed actor should not be reconstructed.~~ Add a flag to disable reconstruction for a killed actor Mar 4, 2020

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

6e3ae45

raulchen reviewed Mar 5, 2020

View reviewed changes

stephanie-wang mentioned this pull request Mar 5, 2020

[core] Ref counting for actor handles #7434

Merged

3 tasks

kfstorm added 2 commits March 6, 2020 09:54

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

6968876

Move Ray.killActor to RayActor.kill

6d27bf3

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

274040e

kfstorm commented Mar 11, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

0767154

kfstorm added 2 commits March 12, 2020 19:06

Fix test

b24df34

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

aab767b

raulchen approved these changes Mar 13, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into java_kill_actor_fix

de8b555

raulchen merged commit d6e8f47 into ray-project:master Mar 13, 2020

raulchen deleted the java_kill_actor_fix branch March 13, 2020 11:10

Conversation

kfstorm commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Feb 27, 2020

Uh oh!

AmplabJenkins commented Feb 27, 2020

Uh oh!

AmplabJenkins commented Feb 27, 2020

Uh oh!

edoakes commented Feb 27, 2020

Uh oh!

jovany-wang commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edoakes commented Feb 28, 2020

Uh oh!

mitar commented Feb 29, 2020

Uh oh!

kfstorm commented Mar 1, 2020

Uh oh!

mitar commented Mar 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edoakes commented Mar 1, 2020

Uh oh!

AmplabJenkins commented Mar 4, 2020

Uh oh!

AmplabJenkins commented Mar 4, 2020

Uh oh!

edoakes commented Mar 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulchen Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 11, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

AmplabJenkins commented Mar 12, 2020

Uh oh!

raulchen commented Mar 13, 2020

Uh oh!

AmplabJenkins commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kfstorm commented Feb 27, 2020 •

edited

Loading

jovany-wang commented Feb 28, 2020 •

edited

Loading

mitar commented Mar 1, 2020 •

edited

Loading

raulchen Mar 5, 2020 •

edited

Loading