Second Part of Internal API Refactor #1326

pcmoritz · 2017-12-15T02:14:52Z

This PR switches the old object IDs with the new ones as a preparation for integrating the GCS APIs.

AmplabJenkins · 2017-12-15T02:31:59Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-15T02:31:59Z

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

AmplabJenkins · 2017-12-15T08:35:15Z

Build finished. Test PASSed.

AmplabJenkins · 2017-12-15T08:35: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/2801/
Test PASSed.

AmplabJenkins · 2017-12-15T08:47:02Z

Build finished. Test PASSed.

AmplabJenkins · 2017-12-15T08:47:02Z

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

AmplabJenkins · 2017-12-15T09:19:22Z

Build finished. Test PASSed.

AmplabJenkins · 2017-12-15T09:19:22Z

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

AmplabJenkins · 2017-12-15T17:41:47Z

Build finished. Test FAILed.

AmplabJenkins · 2017-12-15T17:41:47Z

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

AmplabJenkins · 2017-12-15T18:36:35Z

Build finished. Test FAILed.

AmplabJenkins · 2017-12-15T18:36: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/2806/
Test FAILed.

AmplabJenkins · 2017-12-15T21:23:28Z

Build finished. Test PASSed.

AmplabJenkins · 2017-12-15T21:23:29Z

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

AmplabJenkins · 2017-12-16T02:55:02Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-16T02:55:03Z

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

AmplabJenkins · 2017-12-16T07:09:23Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-16T07:09:23Z

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

AmplabJenkins · 2017-12-20T20:44:43Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-20T20:44:44Z

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

AmplabJenkins · 2017-12-20T23:03:08Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-20T23:03:09Z

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

AmplabJenkins · 2017-12-21T00:20:23Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-21T00:20: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/2872/
Test PASSed.

AmplabJenkins · 2017-12-21T00:30:43Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-21T00:30:44Z

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

AmplabJenkins · 2017-12-21T00:34:18Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-21T00: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/2869/
Test FAILed.

AmplabJenkins · 2017-12-21T00:38:02Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-25T06:54:03Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-25T06:54:04Z

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

AmplabJenkins · 2017-12-25T07:15:52Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-25T07:15:53Z

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

AmplabJenkins · 2017-12-25T07:45:28Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-25T07:45:29Z

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

AmplabJenkins · 2017-12-25T07:49:41Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-25T07:49:41Z

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

AmplabJenkins · 2017-12-25T08:13:29Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-25T08:13:30Z

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

AmplabJenkins · 2017-12-25T08:18:37Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-25T08:18:37Z

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

AmplabJenkins · 2017-12-26T20:49:00Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-26T20:49:00Z

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

robertnishihara

Great work here, this is way cleaner than before!

robertnishihara · 2017-12-26T21:20:14Z

src/common/common.h

- * @return True if the worker IDs are the same and false otherwise.
- */
-bool WorkerID_equal(WorkerID first_id, WorkerID second_id);
+typedef ray::UniqueID WorkerID;


Isn't this already defined in id.h?

actually, in id.h we have ray::WorkerID and ray::DBClientID; as most of the GCS code will get removed, I'm just going to keep this for now.

robertnishihara · 2017-12-26T21:20:29Z

src/common/common.h

- * @return True if the db client ID is equal to nil.
- */
-bool DBClientID_is_nil(ObjectID id);
+typedef ray::UniqueID DBClientID;


Isn't this already defined in id.h?

robertnishihara · 2017-12-26T21:24:09Z

src/common/logging.cc

@@ -79,7 +79,7 @@ void RayLogger_log(RayLogger *logger,

    int status =
        redisAsyncCommand(context, NULL, NULL, formatted_message,
-                          (char *) db->client.id, sizeof(db->client.id));
+                          (char *) db->client.data(), sizeof(db->client));


we should be using C++ style casts everywhere (at least for new code)

let's do this incrementally

robertnishihara · 2017-12-26T21:25:54Z

src/common/state/redis.cc

@@ -672,7 +673,8 @@ void redis_object_table_lookup_callback(redisAsyncContext *c,
    for (size_t j = 0; j < reply->elements; ++j) {
      CHECK(reply->element[j]->type == REDIS_REPLY_STRING);
      DBClientID manager_id;
-      memcpy(manager_id.id, reply->element[j]->str, sizeof(manager_id.id));
+      memcpy(manager_id.mutable_data(), reply->element[j]->str,


We have a decent number of memcpy's in the codebase. Maybe this functionality should be part of the ObjectID class.

This code will go away as soon as the new GCS client code will be merged, and the rest should be modernized piece by piece.

robertnishihara · 2017-12-26T21:27:06Z

src/common/task.cc

@@ -145,23 +145,23 @@ void free_task_builder(TaskBuilder *builder) {
 }

 bool TaskID_equal(TaskID first_id, TaskID second_id) {


The plan is for these equal and is_nil methods to go away (e.g., in a future PR) and just use == and is_nil, right?

robertnishihara · 2017-12-26T21:30:57Z

src/global_scheduler/global_scheduler.cc

-  ARROW_UNUSED(id_string);
+  std::string id_string = client_id.hex();
+  LOG_DEBUG("Local scheduler heartbeat from db_client_id %s",
+            id_string.c_str());


What's the plan for these logging macros down the road? E.g., Arrow uses something more compatible with C++ style logging, which would be nice here so we could avoid the .c_str and not have to worry so much about string formatting.

Yeah, the code is already in ray/util/logging.h and the various components should be modernized as we go along.

robertnishihara · 2017-12-26T21:32:36Z

src/plasma/plasma_manager.h

@@ -59,7 +59,7 @@ void PlasmaManagerState_free(PlasmaManagerState *state);
 * manager.
 */
 void process_transfer(event_loop *loop,
-                      ObjectID object_id,
+                      ray::ObjectID object_id,


not super critical, but all of the ray::ObjectID object_id in this file should be const ray::ObjectID &object_id.

Yeah, let's do this incrementally!

robertnishihara · 2017-12-26T21:37:42Z

src/ray/id.cc

-  const uint8_t *data = rhs.data();
-  while (--i > 0 && data[i] == 255) {
+  const uint8_t *d = data();
+  while (--i > 0 && d[i] == 255) {


I might be misreading this, but doesn't this fail to check d[0]? Also, we don't have any guarantees about the order in which the two boolean clauses are executed, right? But if they are executed out of order then we will have problems.

Good catch!

AmplabJenkins · 2017-12-26T22:06:39Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-12-26T22:06:40Z

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

AmplabJenkins · 2017-12-26T22:50:23Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-12-26T22:50: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/2974/
Test PASSed.

pcmoritz force-pushed the c++-api-2 branch from 944732f to 9bd6c6a Compare December 16, 2017 02:10

more fixes

70a2bb0

pcmoritz added 2 commits December 24, 2017 22:54

fix

d4a8504

fixes

a486826

pcmoritz added 2 commits December 24, 2017 23:25

fix build system

d83f625

fix valgrind

35c9126

fix valgrind

564a73f

robertnishihara reviewed Dec 26, 2017

View reviewed changes

pcmoritz added 2 commits December 26, 2017 13:56

fixes

adfdb7e

more fixes

620a626

robertnishihara approved these changes Dec 27, 2017

View reviewed changes

robertnishihara merged commit 3d224c4 into ray-project:master Dec 27, 2017

robertnishihara deleted the c++-api-2 branch December 27, 2017 00:22

		@@ -145,23 +145,23 @@ void free_task_builder(TaskBuilder *builder) {
		}

		bool TaskID_equal(TaskID first_id, TaskID second_id) {

Second Part of Internal API Refactor #1326

Second Part of Internal API Refactor #1326

Conversation

pcmoritz commented Dec 15, 2017 • edited Loading

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 15, 2017

AmplabJenkins commented Dec 16, 2017

AmplabJenkins commented Dec 16, 2017

AmplabJenkins commented Dec 16, 2017

AmplabJenkins commented Dec 16, 2017

AmplabJenkins commented Dec 20, 2017

AmplabJenkins commented Dec 20, 2017

AmplabJenkins commented Dec 20, 2017

AmplabJenkins commented Dec 20, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 21, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 25, 2017

AmplabJenkins commented Dec 26, 2017

AmplabJenkins commented Dec 26, 2017

robertnishihara 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Dec 26, 2017

AmplabJenkins commented Dec 26, 2017

AmplabJenkins commented Dec 26, 2017

AmplabJenkins commented Dec 26, 2017

pcmoritz commented Dec 15, 2017 •

edited

Loading