Shard Redis. #539

stephanie-wang · 2017-05-10T22:38:02Z

Continued from #509.

AmplabJenkins · 2017-05-10T22:46:12Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-10T22:46: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/728/
Test FAILed.

AmplabJenkins · 2017-05-12T19:05:52Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-12T19:05: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/737/
Test FAILed.

robertnishihara

This looks great! I have a few comments/questions!

robertnishihara · 2017-05-13T00:12:21Z

src/common/net.h

@@ -1,6 +1,9 @@
 #ifndef NET_H
 #define NET_H

+#include <string>
+#include <vector>


These includes are no longer necessary, right?

Or maybe they need to be included in a different file (like redis.cc)?

robertnishihara · 2017-05-13T00:12:49Z

src/common/state/db.h

@@ -4,13 +4,18 @@
 #include "common.h"
 #include "event_loop.h"

+#include <vector>


Do we need this include?

robertnishihara · 2017-05-13T00:34:07Z

src/common/state/redis.cc

- if ((status == REDIS_ERR) || db->context->err) {
- LOG_REDIS_DEBUG(db->context,
- "error in redis_object_table_subscribe_to_notifications");
+ redisAsyncContext *context = get_redis_context(db, object_ids[i]);


Why this change? This is turning a single redis command into potentially a million separate redis commands. Couldn't that be really expensive?

Oh, I realize now that you're doing this because the different requests need to go to different redis shards. However, in the case where we are requesting 1 million objects, this may be too expensive..

Yeah, it is possible to batch everything that goes into a single shard together. Let's do that as an optimization later.

robertnishihara · 2017-05-13T00:37:30Z

src/common/state/redis.cc

@@ -779,6 +903,15 @@ void redis_task_table_test_and_update_callback(redisAsyncContext *c,
 redisReply *reply = (redisReply *) r;
 /* Parse the task from the reply. */
 Task *task = parse_and_construct_task_from_redis_reply(reply);
+ if (task == NULL) {


What does this change do, it seems like it changes the behavior of the system? Wouldn't it make sense to still call the callback and let the caller decide how to handle this case in the callback?

Also, if this is only possible with actor tasks, then we should assert that the task is an actor task.

If the task entry is not found in the table, the callback is not called. Previously, it would crash on the next line since reply->str was NULL.

It should only happen for actor tasks at the moment, but the problem is that we can't tell whether it's an actor task or not without the task entry.

robertnishihara · 2017-05-13T00:43:32Z

src/global_scheduler/global_scheduler.cc

 /* The IP address of the node that this global scheduler is running on. */
 char *node_ip_address = NULL;
 int c;
- while ((c = getopt(argc, argv, "h:r:")) != -1) {
+ while ((c = getopt(argc, argv, "h:r:t:")) != -1) {


The -t is unused, right?

robertnishihara · 2017-05-13T00:44:01Z

src/local_scheduler/local_scheduler.cc

@@ -1193,13 +1192,13 @@ int main(int argc, char *argv[]) {
 char *num_workers_str = NULL;
 int c;
 bool global_scheduler_exists = true;
- while ((c = getopt(argc, argv, "s:r:p:m:ga:h:c:w:n:")) != -1) {
+ while ((c = getopt(argc, argv, "s:r:t:p:m:ga:h:c:w:n:")) != -1) {


The t is unused.

robertnishihara · 2017-05-13T00:44:45Z

src/plasma/plasma_manager.cc

 int c;
- while ((c = getopt(argc, argv, "s:m:h:p:r:")) != -1) {
+ while ((c = getopt(argc, argv, "s:m:h:p:r:t:")) != -1) {


The t is unused.

robertnishihara · 2017-05-13T00:48:17Z

test/runtest.py

@@ -1538,7 +1538,8 @@ def testGlobalStateAPI(self):
 task_table = ray.global_state.task_table()


Let's add another test in runtest.py that tests calling ray.init with num_redis_shards=10 or 20 or some large number and then runs some remote functions and gets the results. I don't think we're testing the changes to the ray.init code path anywhere.

robertnishihara · 2017-05-13T00:50:43Z

I'm looking into the jenkins failure now.

robertnishihara · 2017-05-13T01:24:42Z

Ok, I observed the following crash when running the many_driver_test jenkins test.

STDOUT

/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler[0x40f879]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(_Z41redis_task_table_test_and_update_callbackP17redisAsyncContextPvS1_+0x88)[0x42b658]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(redisProcessCallbacks+0x96)[0x44d9c6]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(aeProcessEvents+0x130)[0x434c20]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(aeMain+0x2b)[0x43503b]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(main+0x2f4)[0x40e924]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f783c860830]
/opt/conda/lib/python2.7/site-packages/ray-0.0.1-py2.7.egg/ray/local_scheduler/../core/src/local_scheduler/local_scheduler(_start+0x29)[0x40ec49]

STDERR

[FATAL] (/ray/src/local_scheduler/local_scheduler.cc:565: errno: Operation now in progress) Check failure: ActorID_equal(TaskSpec_actor_id(spec), NIL_ID)

AmplabJenkins · 2017-05-13T01:30:58Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-13T01:30: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/745/
Test FAILed.

robertnishihara · 2017-05-13T01:40:49Z

Also, the most recent jenkins run failed with https://amplab.cs.berkeley.edu/jenkins/job/Ray-PRB/745/console. I've never seen this error before on jenkins and the jenkins don't seem to be flaky, so it's possible there is a problem.

AmplabJenkins · 2017-05-15T02:37:48Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-15T02:37: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/773/
Test FAILed.

AmplabJenkins · 2017-05-15T02:57:44Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-15T02:57:44Z

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

AmplabJenkins · 2017-05-16T01:04:17Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-16T01:04:18Z

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

AmplabJenkins · 2017-05-16T15:29:55Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-16T15:29: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/793/
Test PASSed.

AmplabJenkins · 2017-05-16T19:05:38Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-16T19:05:47Z

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

AmplabJenkins · 2017-05-16T20:24:19Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-16T20:24: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/796/
Test PASSed.

AmplabJenkins · 2017-05-16T21:09:20Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-16T21:09: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/798/
Test PASSed.

AmplabJenkins · 2017-05-16T21:14:33Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-16T21:14: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/799/
Test PASSed.

robertnishihara · 2017-05-16T21:17:03Z

This PR will break the web UI. That's completely fine because we need to rewrite the web UI anyway.

robertnishihara · 2017-05-18T18:03:21Z

src/common/redis_module/ray_redis_module.cc

+ /* This reply will be received by redis_task_table_update_callback or
+ * redis_task_table_add_task_callback in redis.cc, which will then reissue
+ * the command. */
+ RedisModule_ReplyWithError(ctx, "No subscribers received message.");


This is a bug, forgot a return statement here.

AmplabJenkins · 2017-05-18T18:09:25Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T18:09:26Z

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

AmplabJenkins · 2017-05-18T20:19:33Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T20:19: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/825/
Test PASSed.

shaneknapp · 2017-05-18T20:31:19Z

test this please

AmplabJenkins · 2017-05-18T20:44:53Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-18T20:44:54Z

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

AmplabJenkins · 2017-05-18T21:34:42Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T21:34:43Z

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

AmplabJenkins · 2017-05-18T21:44:42Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T21:44:43Z

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

AmplabJenkins · 2017-05-18T21:54:26Z

Merged build finished. Test FAILed.

AmplabJenkins · 2017-05-18T21:54:26Z

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

AmplabJenkins · 2017-05-18T23:24:01Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T23:24: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/831/
Test PASSed.

AmplabJenkins · 2017-05-18T23:34:39Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-05-18T23:34:40Z

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

stephanie-wang mentioned this pull request May 10, 2017

Port redis sharding to master #509

Closed

robertnishihara reviewed May 13, 2017

View reviewed changes

robertnishihara mentioned this pull request May 15, 2017

[DONTMERGE] Redis sharding #424

Closed

stephanie-wang force-pushed the redis-shards branch from a55bc69 to 4c0a2ce Compare May 16, 2017 07:15

fix

8d91982

robertnishihara reviewed May 18, 2017

View reviewed changes

Fix linting.

449cee6

fix tests

febad19

fix linting

494ad49

pcmoritz force-pushed the redis-shards branch from 72806ef to 494ad49 Compare May 18, 2017 21:39

pcmoritz added 2 commits May 18, 2017 16:13

fix python test

dd2433b

fix linting

c55bc5c

robertnishihara changed the title ~~Use multiple Redis shards to reduce load.~~ Shard Redis. May 19, 2017

pcmoritz merged commit ee08c82 into ray-project:master May 19, 2017

pcmoritz deleted the redis-shards branch May 19, 2017 00:40

This was referenced May 19, 2017

Print the driver stdout/stderr if we fail to decode it in jenkins. #567

Merged

Valgrind error in local scheduler tests. #570

Closed

		@@ -1538,7 +1538,8 @@ def testGlobalStateAPI(self):
		task_table = ray.global_state.task_table()

Shard Redis. #539

Shard Redis. #539

Conversation

stephanie-wang commented May 10, 2017

AmplabJenkins commented May 10, 2017

AmplabJenkins commented May 10, 2017

AmplabJenkins commented May 12, 2017

AmplabJenkins commented May 12, 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

robertnishihara commented May 13, 2017

robertnishihara commented May 13, 2017

AmplabJenkins commented May 13, 2017

AmplabJenkins commented May 13, 2017

robertnishihara commented May 13, 2017

AmplabJenkins commented May 15, 2017

AmplabJenkins commented May 15, 2017

AmplabJenkins commented May 15, 2017

AmplabJenkins commented May 15, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

AmplabJenkins commented May 16, 2017

robertnishihara commented May 16, 2017

Choose a reason for hiding this comment

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

shaneknapp commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017

AmplabJenkins commented May 18, 2017