Add common preprocessing for each request in node manager. #5296

jiangzihao2009 · 2019-07-27T17:37:54Z

What do these changes do?

Add common preprocessing for each request in node manager
Three parts are included in preprocessing, logging, checking heartbeat and checking being killed.
Change submit task request to an asynchronous request.

Related issue number

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-07-27T19:52:42Z

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

AmplabJenkins · 2019-07-27T20:40:02Z

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

zhijunfu

Thanks! I left a few comments.

src/ray/raylet/node_manager.h

src/ray/raylet/node_manager.cc

src/ray/protobuf/raylet.proto

src/ray/raylet/node_manager.cc

raulchen · 2019-07-29T04:56:24Z

src/ray/raylet/node_manager.cc

+  if (expiry_delay > expiry_delay_limit) {
+    heartbeat_timer_.cancel();
+    Heartbeat();
+  }


We need to check the timer in HandleForwardTaskRequest as well. maybe move this to a CheckHeartbeatTimer function.

Theoretically we should add the check for all the messages in raylet? node manager, object manager, and from worker to raylet.

Yes, but except object manager requests, which are handled in background threads.

Why do we also need to check the timer in HandleForwardTaskRequest? The heartbeats are only for workers, not for remote node/object managers, right?

@stephanie-wang because if raylet suddenly receives a huge number of ForwardTask requests, raylet-to-gcs heartbeat will time out as well.

src/ray/raylet/node_manager.cc

raulchen · 2019-07-29T05:10:30Z

Also, can you explain that the main purpose of this PR is to solve the issue that the heartbeat timer will be delayed and makes a node mistakenly marked as dead, when node manager suddenly receives many requests and posts many handlers to the event loop?

AmplabJenkins · 2019-07-29T11:28:43Z

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

AmplabJenkins · 2019-07-30T05:11:23Z

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

AmplabJenkins · 2019-07-30T09:29:48Z

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

AmplabJenkins · 2019-07-31T09:00:03Z

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

AmplabJenkins · 2019-07-31T11:31: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/15832/
Test PASSed.

raulchen

Looks good overall. Just a few small comments.

src/ray/protobuf/raylet.proto

src/ray/raylet/node_manager.cc

raulchen · 2019-07-31T13:45:57Z

src/ray/raylet/node_manager.cc

@@ -735,6 +739,83 @@ void NodeManager::DispatchTasks(
  local_queues_.MoveTasks(assigned_task_ids, TaskState::READY, TaskState::RUNNING);
 }

+std::pair<std::shared_ptr<Worker>, bool> NodeManager::GetWorker(
+    const WorkerID &worker_id) {
+  bool is_worker = true;


instead of returning a pair here, I think it's clearer to put is_worker in Worker class.

Agreed. Also, this should not be a method of NodeManager.

stephanie-wang

This seems like a pretty brittle and at-best temporary solution, but I guess it's okay for now. How feasible would it be to move the heartbeat handling to a separate thread? Is it necessary to use heartbeats for worker-raylet communication?

stephanie-wang · 2019-07-31T17:55:23Z

src/ray/raylet/node_manager.cc

+  if (expiry_delay > expiry_delay_limit) {
+    heartbeat_timer_.cancel();
+    Heartbeat();
+  }


Why do we also need to check the timer in HandleForwardTaskRequest? The heartbeats are only for workers, not for remote node/object managers, right?

src/ray/raylet/node_manager.cc

stephanie-wang · 2019-07-31T17:58:24Z

src/ray/raylet/node_manager.cc

@@ -735,6 +739,83 @@ void NodeManager::DispatchTasks(
  local_queues_.MoveTasks(assigned_task_ids, TaskState::READY, TaskState::RUNNING);
 }

+std::pair<std::shared_ptr<Worker>, bool> NodeManager::GetWorker(
+    const WorkerID &worker_id) {
+  bool is_worker = true;


Agreed. Also, this should not be a method of NodeManager.

src/ray/raylet/node_manager.cc

src/ray/raylet/node_manager.h

pcmoritz · 2019-07-31T19:50:41Z

I also think moving the heartbeats to a different thread is the right solution here. We do need to make sure that it is not possible for the main thread to get stuck and the heartbeat thread to keep sending heartbeats however.

raulchen · 2019-08-01T03:26:25Z

@stephanie-wang @pcmoritz The problem with moving heartbeat to a different thread is that we need to use the gcs client to send heartbeat. However, the gcs client is bound to the io_service in the main thread.

raulchen · 2019-08-01T03:34:51Z

I think the actual problem is that asio doesn't prioritize timers over post handlers (AFAIK, some other event loops, e.g., Python's event loop, do.) Otherwise, it's totally fine to put timer in the event loop.

raulchen · 2019-08-01T04:51:10Z

@pcmoritz @stephanie-wang Since it's not easy to move heartbeat timer to another thread, let's use this PR to unblock the issue for now?

raulchen

LGTM. just one typo

raulchen · 2019-08-01T04:53:57Z

src/ray/raylet/node_manager.h

-  /// Preprocess request from raylet client. This takes care of common processing for each
-  /// request, such as logging, checking heartbeat and whether worker is being killed.
+  /// Preprocess request from raylet client. We will check whether the worker is being
+  /// killed due to exitting of driver. Also we will send the heartbeat to monitor


typo exiting

simon-mo · 2019-08-01T06:07:24Z

I wonder how much fixed overhead does this preprocessing task will create? Can we quickly check how much slower does it create?

AmplabJenkins · 2019-08-01T06:47:25Z

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

AmplabJenkins · 2019-08-01T07:12:45Z

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

raulchen · 2019-08-01T07:35:41Z

@simon-mo checking timer should be very cheap. my local test with your script shows it's even slightly faster than master.

AmplabJenkins · 2019-08-01T10:49:55Z

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

…_pre

stephanie-wang · 2019-08-01T17:49:48Z

After some discussion with @pcmoritz and @simon-mo, let's not merge this PR unless it is really necessary for stability even after #5341 has been merged. For raylet-to-redis heartbeats, the issue has been known for a while and the solution needs to be more comprehensive than this PR. For worker heartbeats, we think the long-term solution is to remove them completely, since it is possible for the raylet to detect worker failures without heartbeats. We're concerned that merging this code will encourage further short-term fixes when really we should concentrate on fixing the core issues.

jiangzihao2009 · 2019-08-02T03:18:00Z

After some discussion with @pcmoritz and @simon-mo, let's not merge this PR unless it is really necessary for stability even after #5341 has been merged. For raylet-to-redis heartbeats, the issue has been known for a while and the solution needs to be more comprehensive than this PR. For worker heartbeats, we think the long-term solution is to remove them completely, since it is possible for the raylet to detect worker failures without heartbeats. We're concerned that merging this code will encourage further short-term fixes when really we should concentrate on fixing the core issues.

At first, this PR is only intended to print log and check whether the job is stopped, which are missing part of PR #5120. We found the heartbeat problem in some test and tried to solve it here. In fact, the root cause can be concluded as a misusing of GRPC. And I think #5341 is a better solution to solve the heartbeat problem.

raulchen · 2019-08-04T03:33:34Z

I think we can revert the heartbeat change, and merge other changes in this PR.

raulchen · 2019-08-05T06:13:47Z

@stephanie-wang heartbeat timer is reverted. I think this PR can be merged. Please let me know if you have other comments.

AmplabJenkins · 2019-08-05T11:27:08Z

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

stephanie-wang

Thanks!

src/ray/raylet/node_manager.cc

AmplabJenkins · 2019-08-06T12:54:22Z

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

AmplabJenkins · 2019-08-06T14:22: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/16040/
Test FAILed.

jiangzihao2009 added 6 commits July 25, 2019 17:55

add preprocess

736a01d

Merge remote-tracking branch 'ray/master' into grpc_pre

4e08e06

finish preprocess

43682f6

chang log level

33e113a

fix

73c67e9

small fix

ebbdb05

raulchen requested review from ericl, stephanie-wang and raulchen July 29, 2019 03:55

zhijunfu reviewed Jul 29, 2019

View reviewed changes

raulchen reviewed Jul 29, 2019

View reviewed changes

fix comments

59ec69d

jiangzihao2009 added 2 commits July 30, 2019 11:12

fix

7a752a6

Merge remote-tracking branch 'origin/master' into grpc_pre

e1ffc10

raulchen mentioned this pull request Jul 30, 2019

Perfomance degradation in raylet task submission #5310

Closed

fix

80adcc7

fix

157d333

jiangzihao2009 added 4 commits July 31, 2019 11:11

add log

4176e16

Merge remote-tracking branch 'origin/master' into grpc_pre

c7404a8

merge master

31d318d

fix conflict

d011f15

raulchen reviewed Jul 31, 2019

View reviewed changes

stephanie-wang requested changes Jul 31, 2019

View reviewed changes

jiangzihao2009 added 2 commits August 1, 2019 11:43

fix comment

1897e6b

typo

f12f64d

raulchen approved these changes Aug 1, 2019

View reviewed changes

lint and typo

12939a2

jiangzihao2009 added 2 commits August 1, 2019 22:53

Merge branch 'master' of https://github.com/ray-project/ray into grpc…

f4d07a5

…_pre

typo

3074eb7

remove heartbeat check

4e4bf95

stephanie-wang approved these changes Aug 5, 2019

View reviewed changes

src/ray/raylet/node_manager.cc Outdated Show resolved Hide resolved

src/ray/raylet/node_manager.cc Show resolved Hide resolved

jiangzihao2009 added 2 commits August 6, 2019 12:36

rename macro to preprocess_worker_request

bd4352d

fix macro

2615b37

raulchen merged commit 02c5d2b into ray-project:master Aug 6, 2019

raulchen deleted the grpc_pre branch August 6, 2019 12:49

pcmoritz mentioned this pull request Aug 13, 2019

Revert raylet to worker GRPC communication back to asio #5450

Merged

1 task

Add common preprocessing for each request in node manager. #5296

Add common preprocessing for each request in node manager. #5296

Conversation

jiangzihao2009 commented Jul 27, 2019 • edited Loading

What do these changes do?

Related issue number

Linter

AmplabJenkins commented Jul 27, 2019

AmplabJenkins commented Jul 27, 2019

zhijunfu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulchen commented Jul 29, 2019

AmplabJenkins commented Jul 29, 2019

AmplabJenkins commented Jul 30, 2019

AmplabJenkins commented Jul 30, 2019

AmplabJenkins commented Jul 31, 2019

AmplabJenkins commented Jul 31, 2019

raulchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanie-wang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz commented Jul 31, 2019

raulchen commented Aug 1, 2019

raulchen commented Aug 1, 2019

raulchen commented Aug 1, 2019

raulchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon-mo commented Aug 1, 2019

AmplabJenkins commented Aug 1, 2019

AmplabJenkins commented Aug 1, 2019

raulchen commented Aug 1, 2019

AmplabJenkins commented Aug 1, 2019

stephanie-wang commented Aug 1, 2019

jiangzihao2009 commented Aug 2, 2019

raulchen commented Aug 4, 2019

raulchen commented Aug 5, 2019

AmplabJenkins commented Aug 5, 2019

stephanie-wang left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Aug 6, 2019

AmplabJenkins commented Aug 6, 2019

jiangzihao2009 commented Jul 27, 2019 •

edited

Loading