[XRay] Raylet node and object manager unification/backend redesign. #1640

atumanov · 2018-03-02T08:06:44Z

What do these changes do?

This PR is the first pass at redesigning and refactoring the Ray backend responsible for task, actor, and object management. We unify this functionality in a single new component, called Raylet, which is intended to combine the functionality of the current local scheduler and the object manager. The rewrite aims to follow object-oriented design, taking the opportunity to convert the backend to pure C++.

Significant changes:

All backend event loops are to use boost::asio, with type safety.
New component (Raylet) to replace local scheduler and object manager.
Global scheduler is no longer assumed to be present.
Moving GCS off the critical path, to fast-track common task placement patterns.
All backend code follows OO design and is converted to pure C++.

Specific functionality in this PR:

Maintain a pool of local workers.
Schedule and assign a task based on current node resources.
Schedule a task with local and remote object dependencies (no failure handling).
Push an object from one object manager to another (no failure handling, single-threaded, mocked object directory).
Pull an object from one object manager to another (no failure handling, single-threaded, mocked object directory).

Planned functionality not in this PR:

Implement direct node manager to node manager communication for task placement.
Turn on and use Lineage Cache on each node manager to locally cache task and object control information as well as forward it to a remote node manager for task execution without pushing it to GCS.

robertnishihara · 2018-03-02T08:40:03Z

cc @imzhenyu

AmplabJenkins · 2018-03-02T09:10:13Z

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

AmplabJenkins · 2018-03-03T01:29:14Z

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

pcmoritz · 2018-03-03T19:47:19Z

src/ray/gcs/tables.h

+  /// Get a client's information from the cache.
+  ///
+  /// @param client The client to get information about.
+  const ClientInformation &GetClientInformation(ClientID client);


const ClientID& client

pcmoritz · 2018-03-03T19:50:40Z

src/ray/object_manager/object_directory.h

@@ -0,0 +1,134 @@
+#ifndef RAY_OBJECTDIRECTORY_H


RAY_OBJECT_DIRECTORY_H

pcmoritz · 2018-03-03T19:51:23Z

src/ray/object_manager/object_directory.h

+ private:
+
+  /// Reference to the gcs client.
+  std::shared_ptr<GcsClient> gcs_client;


gcs_client_

pcmoritz · 2018-03-03T19:52:01Z

src/ray/object_manager/object_directory.cc

+ObjectDirectory::~ObjectDirectory() = default;
+
+ObjectDirectory::ObjectDirectory(std::shared_ptr<GcsClient> gcs_client){
+  this->gcs_client = gcs_client;


after gcs_client -> gcs_client_, can remove the this-> everywhere.

pcmoritz · 2018-03-03T20:02:30Z

src/ray/object_manager/object_directory.h

+  /// \param object_id The object id that was put into the store.
+  /// \param client_id The client id corresponding to this node.
+  /// \return Status of whether this method succeeded.
+  virtual ray::Status ObjectAdded(const ObjectID &object_id,


maybe call this ReportObjectAdded?

pcmoritz · 2018-03-03T20:03:43Z

src/ray/object_manager/object_manager.cc

+                             shared_ptr<ray::GcsClient> gcs_client)
+  : od(new ObjectDirectory(gcs_client)),
+    work_(io_service_) {
+  this->store_client_ = unique_ptr<ObjectStoreClient>(new ObjectStoreClient(io_service, config.store_socket_name));


remove the this->

pcmoritz · 2018-03-03T20:18:05Z

src/ray/raylet/actor.cc

+
+ActorInformation::~ActorInformation() {}
+
+const ActorID& ActorInformation::GetActorId() const {


be consistent with Id and ID

pcmoritz · 2018-03-03T20:33:05Z

src/ray/raylet/node_manager.h

+
+namespace ray {
+
+class NodeManager : public ClientManager<boost::asio::local::stream_protocol> {


Rename this to LocalScheduler? And "Raylet" is what we used to call the "NodeManager".

Hmm, I think this was the way we originally had it but I'm not sure now what the reasoning was for changing it. @atumanov?

(1) The notion of "local" scheduler is being outdated with this rewrite, because there's no "global" scheduler, which makes the term ambiguous and suggestive of the fact there's something other than the "local" scheduler.
(2) I think we might simply be used to the term and, naturally, have affinity for calling this component as it was named before. If we take a step back, I think this component is doing more than just task scheduling. Scheduling is only part of the NodeManager's responsibility. In fact, the scheduler is a component inside the NodeManager, periodically invoked to make scheduling decisions.

I felt like the NodeManager is more appropriate, as a more general and descriptive term capturing the fact that this component does more than just scheduling. It performs functions related to the management of the local node. Raylet is the name of the whole process that includes node management, object management, etc.

Let me know if this works for you, @pcmoritz .

AmplabJenkins · 2018-03-03T22:09:13Z

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

AmplabJenkins · 2018-03-03T22:34:19Z

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

AmplabJenkins · 2018-03-07T04:12: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/4163/
Test FAILed.

AmplabJenkins · 2018-03-07T04:32:21Z

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

AmplabJenkins · 2018-03-07T17:48:49Z

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

AmplabJenkins · 2018-03-07T19:04:38Z

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

AmplabJenkins · 2018-03-07T20:08:27Z

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

robertnishihara · 2018-03-07T20:33:22Z

src/ray/object_manager/object_directory.cc

+  ray::Status status_code = ray::Status::OK();
+  if (existing_requests_.count(object_id) == 0) {
+    existing_requests_[object_id] = ODCallbacks({success_cb, fail_cb});
+    ;


does this semicolon do anything?

robertnishihara · 2018-03-07T20:34:07Z

src/ray/gcs/tables.h

  ///        becomes available.
-  /// @param done_callback Callback to be called when subscription is installed.


Minor, but "becomes available" should be unindented (other places in this file as well)

robertnishihara · 2018-03-07T20:35:35Z

src/ray/object_manager/object_directory.cc

+
+ray::Status ObjectDirectory::GetInformation(const ClientID &client_id,
+                                            const InfoSuccessCallback &success_cb,
+                                            const InfoFailureCallback &fail_cb) {


I'd prefer success_callback and fail_callback here (and other places)

robertnishihara · 2018-03-07T20:36:02Z

src/ray/object_manager/object_directory.cc

+
+ray::Status ObjectDirectory::GetLocationsComplete(
+    const ray::Status &status, const ObjectID &object_id,
+    const std::vector<RemoteConnectionInfo> &v) {


Call this something other than v?

robertnishihara · 2018-03-07T20:36:45Z

src/ray/object_manager/object_directory.h

+namespace ray {
+
+struct RemoteConnectionInfo {
+  RemoteConnectionInfo(const ClientID &id, const std::string &ipaddr, uint16_t portnum)


We're using ip_address in most places n the codebase. Also portnum -> port_num

robertnishihara · 2018-03-07T20:40:41Z

src/ray/raylet/mock_gcs_client.cc

+}
+
+ray::Status ClientTable::Add(const ClientID &client_id, const std::string &ip,
+                             uint16_t port, DoneCallback done) {


done -> done_callback

robertnishihara · 2018-03-07T20:41:22Z

src/ray/raylet/raylet.cc

+Raylet::Raylet(boost::asio::io_service& io_service,
+               const std::string &socket_name,
+               const ResourceSet &resource_config,
+               const ObjectManagerConfig &om_config,


om_config -> object_manager_config

robertnishihara · 2018-03-07T20:43:17Z

src/ray/raylet/scheduling_resources.h

+  bool SubtractResources(const ResourceSet &other);
+
+  /// Return the capacity value associated with the specified resource.
+  /// \param[in] resource_name: Resource name for which capacity is requested.


we aren't using \param[in] in very many places, what does it mean?

robertnishihara · 2018-03-07T20:43:57Z

src/ray/raylet/task_execution_spec.h

+  /// Create a task execution specification.
+  ///
+  /// \param execution_dependencies The task's dependencies, determined at
+  ///        execution time.


unindent (also other places in this file)

robertnishihara · 2018-03-07T20:44:20Z

src/ray/raylet/task_spec.h

+#include <cstddef>
+#include <string>
+#include <vector>
+#include <unordered_map>


u comes before v :)

AmplabJenkins · 2018-03-07T22:34: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/4183/
Test PASSed.

AmplabJenkins · 2018-03-07T22:51:16Z

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

AmplabJenkins · 2018-03-08T05:59:34Z

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

AmplabJenkins · 2018-03-08T06:04:56Z

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

AmplabJenkins · 2018-03-08T06:10:24Z

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

+1 in person

atumanov requested review from pcmoritz, robertnishihara, concretevitamin, elibol and stephanie-wang March 2, 2018 08:06

pcmoritz reviewed Mar 3, 2018

View reviewed changes

src/ray/object_manager/object_directory.h Outdated

@@ -0,0 +1,134 @@

#ifndef RAY_OBJECTDIRECTORY_H

Copy link

Contributor

pcmoritz Mar 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RAY_OBJECT_DIRECTORY_H

pcmoritz reviewed Mar 3, 2018

View reviewed changes

atumanov and others added 12 commits March 3, 2018 15:52

directory for raylet

310486e

some initial class scaffolding -- in progress

6f6bd22

node_manager build code and test stub files.

c5385b2

class scaffolding for resources, workers, and the worker pool

2e81d21

raylet policy and queue - wip checkpoint

ac63413

Node manager server loop

0a90b2b

fix dependencies

4997957

add gen_nm_fbs as target.

85c3e97

object manager build, stub, and test code.

92570ac

Start integrating WorkerPool into node manager

4e1a39c

fix build on mac

4cea2b0

adding LsResources boilerplate

f7eabfe

consistent caps and naming; misc style

0297fb2

Merge branch 'master' into xray

f14db4a

cleaning up client connection + other stylistic fixes

d9d080f

cmath, std::nan

afda46c

robertnishihara reviewed Mar 7, 2018

View reviewed changes

atumanov added 2 commits March 7, 2018 13:35

more style polish: OM, Raylet, gcs tables

a7a9154

removing sandbox (moved to ray-project/sandbox)

fddea12

atumanov added 4 commits March 7, 2018 21:01

raylet linting

9d4eeeb

object manager linting

90aca41

gcs linting

27c547d

all other linting

3825398

pcmoritz self-requested a review March 8, 2018 19:54

pcmoritz approved these changes Mar 8, 2018

View reviewed changes

stephanie-wang approved these changes Mar 8, 2018

View reviewed changes

elibol approved these changes Mar 8, 2018

View reviewed changes

stephanie-wang merged commit 91464a5 into master Mar 8, 2018

robertnishihara deleted the xray branch March 8, 2018 20:57


		ActorInformation::~ActorInformation() {}

		const ActorID& ActorInformation::GetActorId() const {


		namespace ray {

		class NodeManager : public ClientManager<boost::asio::local::stream_protocol> {

		/// becomes available.
		/// @param done_callback Callback to be called when subscription is installed.

[XRay] Raylet node and object manager unification/backend redesign. #1640

[XRay] Raylet node and object manager unification/backend redesign. #1640

Conversation

atumanov commented Mar 2, 2018

What do these changes do?

Significant changes:

Specific functionality in this PR:

Planned functionality not in this PR:

robertnishihara commented Mar 2, 2018

AmplabJenkins commented Mar 2, 2018

AmplabJenkins commented Mar 3, 2018

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

Choose a reason for hiding this comment

pcmoritz Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atumanov Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Mar 3, 2018

AmplabJenkins commented Mar 3, 2018

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 7, 2018

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

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

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 7, 2018

AmplabJenkins commented Mar 8, 2018

AmplabJenkins commented Mar 8, 2018

AmplabJenkins commented Mar 8, 2018

pcmoritz Mar 3, 2018 •

edited

Loading

atumanov Mar 5, 2018 •

edited

Loading