Local scheduler filters out dead clients during reconstruction #1182

stephanie-wang · 2017-11-02T23:46:52Z

When there are many keys in Redis, monitor cleanup of the task and object tables is very slow. This leads to high latencies (minutes or more) during reconstruction.

This removes that latency by allowing the local scheduler to listen to notifications about dead clients from the db client table and filter out lost tasks/objects itself, instead of waiting for the monitor. This includes the following changes:

Expose a call to the db_client_table that allows a client to build a cache of the other database clients.
Add an option to specify a local scheduler ID in the task_table_test_and_update call. If this option is set, then the current local scheduler ID is tested before doing the task update.
During reconstruction, if the local scheduler is not able to update a task to TASK_STATUS_RECONSTRUCTING at first, then it checks if the currently assigned local scheduler is dead. If yes, then the local scheduler tries the test_and_update again.

AmplabJenkins · 2017-11-02T23:59:30Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-02T23:59:31Z

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

AmplabJenkins · 2017-11-03T00:29:38Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-03T00:29: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/2255/
Test FAILed.

AmplabJenkins · 2017-11-03T00:53:58Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-03T00:53:58Z

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

AmplabJenkins · 2017-11-03T01:32:20Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-03T01:32:20Z

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

AmplabJenkins · 2017-11-03T22:51:34Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-03T22:51:35Z

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

robertnishihara

Nice work here, I left some comments/questions. My primary question is the one about the two code paths for reconstruction.

Also, this PR does a lot more than what is stated in the PR description. Could you add some more details to the description? Including why we need to change the task_table_test_and_update API?

robertnishihara · 2017-11-06T23:14:47Z

src/common/test/db_tests.cc

+  CHECK(manager_ids.size() == 2);
+  const std::vector<std::string> managers =
+      db_client_table_get_ip_addresses(db, manager_ids);
+  if (sscanf(managers.at(0).c_str(), "%15[0-9.]:%5[0-9]", received_addr1,


Does it make sense to use this function?

ray/src/common/net.cc

Line 9 in 7215f7d

int parse_ip_addr_port(const char *ip_addr_port, char *ip_addr, int *port) {

Ah yes, thanks! I think this is legacy.

robertnishihara · 2017-11-06T23:15:04Z

src/common/test/db_tests.cc

             received_port1) != 2) {
    CHECK(0);
  }
-  if (sscanf(manager_vector.at(1).c_str(), "%15[0-9.]:%5[0-9]", received_addr2,
+  if (sscanf(managers.at(1).c_str(), "%15[0-9.]:%5[0-9]", received_addr2,


Same comment here.

robertnishihara · 2017-11-06T23:35:37Z

src/common/state/db_client_table.cc

+}
+
+DBClient db_client_table_cache_get(DBHandle *db_handle, DBClientID client_id) {
+  CHECK(!ObjectID_is_nil(client_id));


This should be DBClientID_is_nil

robertnishihara · 2017-11-06T23:41:03Z

src/plasma/plasma_manager.cc

@@ -464,7 +458,7 @@ PlasmaManagerState *PlasmaManagerState_init(const char *store_socket_name,
    db_connect_args[1] = store_socket_name;
    db_connect_args[2] = "manager_socket_name";
    db_connect_args[3] = manager_socket_name;
-    db_connect_args[4] = "address";
+    db_connect_args[4] = "aux_address";


Why this terminology change?

This is the address of the plasma manager, so doesn't it make sense to call it address (or perhaps manager_address)?

Mostly so that we can have just one field for both the local scheduler and the plasma manager, instead of two. I guess it's not totally necessary, but it makes the parsing on the Redis module a little easier if we only have to look for one string. I think calling it manager_address instead probably makes sense though.

robertnishihara · 2017-11-06T23:45:18Z

src/local_scheduler/local_scheduler.cc

+   * avoid waiting for the heartbeat timeout. */
+  if (state->db != NULL) {
+    local_scheduler_table_disconnect(state->db);
+  }


At a high level, is there some danger that this change introduces two code separate code paths? One that gets used when we kill the local schedulers manually and the exit gracefully, and one that gets used when things die unexpectedly and ungracefully? The first code path is what happens in all of our tests, and the second code path is what would happen in real world usage?

Do you see a resolution to this? What are your thoughts about it?

Yes, that's true. When I wrote the local_scheduler_table_disconnect call, I tried to match it to the current codepath as much as possible, i.e. the global scheduler sets the last heartbeat to long ago instead of deleting the local scheduler immediately. Therefore, it should be as if we just sped up the time to timeout the local scheduler, but it's hard to say if we covered all corner cases.

Probably the resolution to this is to do some more thorough testing with a live cluster, where we abruptly kill nodes. This is on the backlog.

Is the only point of this to speed up the tests? If so, another alternative is to just use a smaller number of missed heartbeats to trigger cleanup in the tests. If you think that makes sense we can do it in a separate PR.

It's also for cases where the local scheduler doesn't exit fatally. I think it's useful to keep it here so that we can do things like scale down a live cluster without having to wait for the full heartbeat timeout.

robertnishihara · 2017-11-07T00:04:00Z

src/common/state/object_table.h

@@ -18,14 +18,14 @@
 typedef void (*object_table_lookup_done_callback)(
    ObjectID object_id,
    bool never_created,
-    const std::vector<std::string> &manager_vector,
+    const std::vector<DBClientID> &manager_ids,


This is a great change.

robertnishihara · 2017-11-07T00:26:53Z

src/local_scheduler/local_scheduler.cc

+    /* The test-and-set failed. The task is either: (1) not finished yet, (2)
+     * lost, but not yet updated, or (3) already being reconstructed. */
+    DBClientID current_local_scheduler_id = Task_local_scheduler(task);
+    if (!ObjectID_is_nil(current_local_scheduler_id)) {


DBClientID_is_nil

robertnishihara · 2017-11-07T00:27:01Z

src/local_scheduler/local_scheduler.cc

+    /* The test-and-set failed. The task is either: (1) not finished yet, (2)
+     * lost, but not yet updated, or (3) already being reconstructed. */
+    DBClientID current_local_scheduler_id = Task_local_scheduler(task);
+    if (!ObjectID_is_nil(current_local_scheduler_id)) {


DBClientID_is_nil

robertnishihara · 2017-11-07T00:28:41Z

src/local_scheduler/local_scheduler.cc

+    if (!ObjectID_is_nil(current_local_scheduler_id)) {
+      DBClient current_local_scheduler =
+          db_client_table_cache_get(state->db, current_local_scheduler_id);
+      if (!current_local_scheduler.is_insertion) {


The is_insertion terminology doesn't really make sense in this context. E.g., it's a property of an update to the DBClient, not of the DBClient itself, right?

Yeah, perhaps we can change it to is_alive?

robertnishihara · 2017-11-07T01:06:56Z

src/common/redis_module/ray_redis_module.cc

+      /* A test local scheduler ID was provided. Test whether it is equal to
+       * the current local scheduler ID before performing the update. */
+      RedisModuleString *test_local_scheduler_id = argv[5];
+      if (RedisModule_StringCompare(current_local_scheduler_id,


Can we do this comparison before the if (current_state_integer & test_state_bitmask) { line? It seems a little confusing to start with update = false, then set update = true, and then set it back to update = false.

Ah yes, let me clean up this section...

robertnishihara · 2017-11-09T01:41:29Z

I introduced some conflicts in #1192 (which moved configuration constants into a separate file). I think it should just involved renaming constants.

…trings

…ring

…gers

AmplabJenkins · 2017-11-09T06:32:32Z

Build finished. Test PASSed.

AmplabJenkins · 2017-11-09T06:32:33Z

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

AmplabJenkins · 2017-11-09T06:36:57Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-09T06:36:57Z

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

AmplabJenkins · 2017-11-09T06:47:29Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-11-09T06:47:30Z

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

AmplabJenkins · 2017-11-09T20:52:50Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-11-09T20:52:51Z

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

stephanie-wang force-pushed the local-scheduler-dead-clients branch from 81a5a43 to 9924066 Compare November 3, 2017 00:16

robertnishihara reviewed Nov 7, 2017

View reviewed changes

stephanie-wang force-pushed the local-scheduler-dead-clients branch from 8faf14d to 4587c34 Compare November 9, 2017 06:08

stephanie-wang added 13 commits November 8, 2017 22:14

Object table lookup returns vector of DBClientID instead of address s…

577dfe0

…trings

Add node IP address to DBClient notification

0aece21

DB client cache stores entire DB client, convert addresses to std::st…

4ad27a3

…ring

get cached db client returns the client

847cb7f

Expose a call to initialize the redis cache

5169ece

Local scheduler filters out dead clients during reconstruction

3df30f8

Remove node ip address from dbclient, use aux_address for plasma mana…

84fac36

…gers

Get entire db client entry when not found in cache

2a92941

Fix common tests

dd2aa36

Fix address in tests

d3b4705

Push error to driver if driver task did the put

9528fb1

Address Robert's comments and cleanup

04239c2

Remove unused Redis command

cfccebd

stephanie-wang force-pushed the local-scheduler-dead-clients branch from 4587c34 to cfccebd Compare November 9, 2017 06:14

Fix db test

ba9a1ea

robertnishihara approved these changes Nov 10, 2017

View reviewed changes

robertnishihara merged commit 07f0532 into ray-project:master Nov 10, 2017

robertnishihara deleted the local-scheduler-dead-clients branch November 10, 2017 19:29

Local scheduler filters out dead clients during reconstruction #1182

Local scheduler filters out dead clients during reconstruction #1182

Conversation

stephanie-wang commented Nov 2, 2017 • edited Loading

AmplabJenkins commented Nov 2, 2017

AmplabJenkins commented Nov 2, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 2017

AmplabJenkins commented Nov 3, 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

Choose a reason for hiding this comment

robertnishihara commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

AmplabJenkins commented Nov 9, 2017

stephanie-wang commented Nov 2, 2017 •

edited

Loading