-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Java] Support direct actor call in Java worker #5504
Conversation
Test PASSed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test FAILed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test PASSed. |
4f953f7
to
3586456
Compare
Test PASSed. |
Test PASSed. |
Test PASSed. |
Test FAILed. |
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Looks good overall. We can merge this when some small comments are addressed.
@@ -10,26 +10,31 @@ | |||
|
|||
public static final int NO_RECONSTRUCTION = 0; | |||
public static final int INFINITE_RECONSTRUCTIONS = (int) Math.pow(2, 30); | |||
public static final boolean DEFAULT_IS_DIRECT_CALL = "1" | |||
.equals(System.getenv("ACTOR_CREATION_OPTIONS_DEFAULT_IS_DIRECT_CALL")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document that this env var is only used for tests. Users should use setIsDirectCall
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -42,13 +47,20 @@ public Builder setMaxReconstructions(int maxReconstructions) { | |||
return this; | |||
} | |||
|
|||
// Since direct call is not fully supported yet, users are not allowed to set the option to true. | |||
// TODO (kfstorm): uncomment when direct call is ready. | |||
// public Builder setIsDirectCall(boolean isDirectCall) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the issue number to the TODO comment?
src/ray/core_worker/context.h
Outdated
@@ -26,6 +26,8 @@ class WorkerContext { | |||
|
|||
const ActorID &GetCurrentActorID() const; | |||
|
|||
bool IsDirectCallActor() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool IsDirectCallActor() const; | |
bool CurrentActorUseDirectCall() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/ray/raylet/actor_registration.cc
Outdated
const auto actor_handle_id = task->GetTaskSpecification().ActorHandleId(); | ||
const auto dummy_object = task->GetTaskSpecification().ActorDummyObject(); | ||
// Extend its frontier to include the most recent task. | ||
// Note(hchen): this is needed because this method is called before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Note(hchen): this is needed because this method is called before | |
// NOTE(hchen): For non-direct-call actors, this is needed because this method is called before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/ray/raylet/actor_registration.cc
Outdated
// Extend its frontier to include the most recent task. | ||
// Note(hchen): this is needed because this method is called before | ||
// `FinishAssignedTask`, which will be called when the worker tries to fetch | ||
// the next task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the next task. | |
// the next task. For direct-call actors, checkpoint data doesn't contain frontier info, so we don't need to do `ExtendFrontier` here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Test PASSed. |
Test FAILed. |
Test PASSed. |
Test FAILed. |
Test PASSed. |
Why are these changes needed?
Worker/driver can submit task directly to an actor without going through raylet for better performance.
What do these changes do?
#5183 introduced direct actor call in core worker as a transport. This PR adds the option
isDirectCall
inActorCreationOptoins
to make it possible to turn on direct actor call mode for actors.TODO:
Related issue number
#5029
Linter
scripts/format.sh
to lint the changes in this PR.