[xray] Put GCS data into the redis data shard #2298

pcmoritz · 2018-06-25T02:22:10Z

This moves the data for the new GCS into a separate data shard. It also changes the RAY_NEW_GCS codepath to use XRay (the codepath was only there for development until XRay is ready).

eric-jj · 2018-06-25T03:02:36Z

src/ray/gcs/redis_context.cc

+    freeReplyObject(reply);
+    usleep(RayConfig::instance().redis_db_connect_wait_milliseconds() * 1000);
+    num_attempts++;
+    continue;


remove continue; it is useless here.

AmplabJenkins · 2018-06-25T03:03: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/6239/
Test FAILed.

eric-jj · 2018-06-25T03:11:36Z

src/ray/gcs/redis_context.cc

+  while (num_attempts < RayConfig::instance().redis_db_connect_retries()) {
+    // Try to read the number of Redis shards from the primary shard. If the
+    // entry is present, exit.
+    reply = reinterpret_cast<redisReply *>(redisCommand(context, "GET NumRedisShards"));


Add a function std::unique_ptr< redisReply, std::function<void(redisReply*)>> ExecutionCommand(context, const char* command) {
return std::unique_ptr< redisReply, std::function<void(redisReply*)>>(reinterpret_cast<redisReply *>(redisCommand(context, command)), [](redisReply *reply) {freeReplyObject(reply);});
}
to avoid manually call freeReplyObject(reply);

eric-jj · 2018-06-25T03:14:58Z

src/ray/gcs/redis_context.cc

@@ -133,6 +135,71 @@ RedisContext::~RedisContext() {
  }
 }

+void GetRedisShards(redisContext *context, std::vector<std::string> *addresses,


static void GetRedisShards to avoid polluted global namespace.

AmplabJenkins · 2018-06-25T03:18:09Z

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

AmplabJenkins · 2018-06-25T03:20: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/6236/
Test PASSed.

robertnishihara · 2018-06-25T18:47:44Z

src/ray/gcs/client.h

@@ -37,7 +37,7 @@ class RAY_EXPORT AsyncGcsClient {
  /// \param address The GCS IP address.
  /// \param port The GCS port.
  /// \return Status.
-  Status Connect(const std::string &address, int port);
+  Status Connect(const std::string &address, int port, bool sharding);


document the argument

robertnishihara · 2018-06-25T18:49:06Z

src/ray/gcs/redis_context.cc

@@ -133,7 +135,71 @@ RedisContext::~RedisContext() {
  }
 }

-Status RedisContext::Connect(const std::string &address, int port) {
+static void GetRedisShards(redisContext *context, std::vector<std::string> *addresses,


There's some overlap with #2281. @heyucongtom can you take a look?

We are both doing a translation (from old ray get_redis_shards to new) here. I think the major difference is that
[+] #2298 add an use_sharding bool to indicate if we are using sharding.
[-] #2281 defaultly using all shards.
[-] #2281 modifies table.cc so that when tables do the work, they randomly select a redis shard by the ID.
[-] #2281 attaches all contexts to event loop, while only connecting the main context (I guess it may be wrong: since tests are giving timeouts.)

The same thing is: we are writing the exactly same thing in getRedisShards function

Sounds right. This PR is actually really doing sharding. It's just creating a single extra shard (as opposed to an arbitrary number of additional shards).

So I am guessing that there is some overlap(having sent out a msg on Slack on this), in which #2281 focuses more on distributing workload for table’s operations.

There should be little or no conflicts, though.

robertnishihara · 2018-06-25T18:50:28Z

src/ray/gcs/redis_context.cc

@@ -133,7 +135,71 @@ RedisContext::~RedisContext() {
  }
 }

-Status RedisContext::Connect(const std::string &address, int port) {
+static void GetRedisShards(redisContext *context, std::vector<std::string> *addresses,
+                    std::vector<int> *ports) {


Cleaner to pass in as std::vector<int> &ports I think.

robertnishihara · 2018-06-25T18:50:51Z

src/ray/gcs/redis_context.cc

+                           std::vector<int> *ports) {
+  // Get the total number of Redis shards in the system.
+  int num_attempts = 0;
+  redisReply *reply = NULL;


AmplabJenkins · 2018-06-25T19:02: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/6254/
Test FAILed.

AmplabJenkins · 2018-06-25T19:14: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/6255/
Test FAILed.

AmplabJenkins · 2018-06-25T20:02:09Z

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

AmplabJenkins · 2018-06-26T03:10: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/6267/
Test PASSed.

AmplabJenkins · 2018-06-26T08:46: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/6278/
Test PASSed.

AmplabJenkins · 2018-06-26T08:57: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/6281/
Test FAILed.

AmplabJenkins · 2018-06-26T09:00: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/6279/
Test PASSed.

AmplabJenkins · 2018-06-27T01:21: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/6303/
Test PASSed.

AmplabJenkins · 2018-06-28T10:15:52Z

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

AmplabJenkins · 2018-06-28T10:31: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/6361/
Test PASSed.

AmplabJenkins · 2018-06-28T11:08: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/6362/
Test FAILed.

concretevitamin · 2018-06-28T18:57:53Z

In the latest travis build for this PR, Build #751, I do not see the quoted error. Just restarted that build let's see what's going on.

concretevitamin · 2018-06-28T18:58:23Z

src/global_scheduler/global_scheduler.cc

@@ -142,8 +142,9 @@ GlobalSchedulerState *GlobalSchedulerState_init(event_loop *loop,
  db_attach(state->db, loop, false);

  RAY_CHECK_OK(state->gcs_client.Connect(std::string(redis_primary_addr),
-                                         redis_primary_port));
+                                         redis_primary_port, true));


In all C++ files in this PR, please use /*sharding=*/true (or false, correspondingly).

concretevitamin · 2018-06-28T18:59:08Z

src/ray/gcs/client.h

  std::shared_ptr<RedisContext> context_;
  std::unique_ptr<RedisAsioClient> asio_async_client_;
  std::unique_ptr<RedisAsioClient> asio_subscribe_client_;
-
+  // The following context writes everything to the primary shard
+  std::shared_ptr<RedisContext> auxiliary_context_;


Is there a reason to call this auxiliary instead of primary?

AmplabJenkins · 2018-06-29T04:38:01Z

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

AmplabJenkins · 2018-06-30T20:57: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/6401/
Test FAILed.

AmplabJenkins · 2018-06-30T21:01: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/6402/
Test PASSed.

AmplabJenkins · 2018-06-30T21:06: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/6403/
Test PASSed.

AmplabJenkins · 2018-06-30T21:43:17Z

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

AmplabJenkins · 2018-06-30T22:49:51Z

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

AmplabJenkins · 2018-06-30T23:45:58Z

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

AmplabJenkins · 2018-07-01T00:08:46Z

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

ray-project#2298)

Basically a re-implementation of #2281, with modifications of #2298 (A fix of #2334, for rebasing issues.). [+] Implement sharding for gcs tables. [+] Keep ClientTable and ErrorTable managed by the primary_shard. TaskTable is managed by the primary_shard for now, until a good hashing for tasks is implemented. [+] Move AsyncGcsClient's initialization into Connect function. [-] Move GetRedisShard and bool sharding from RedisContext's connect into AsyncGcsClient. This may make the interface cleaner.

pcmoritz added 4 commits June 24, 2018 19:15

use redis data shard for XRay

0d12ff3

fix

15794b1

linting

408a349

fix test

fd19650

eric-jj reviewed Jun 25, 2018

View reviewed changes

pcmoritz added 2 commits June 25, 2018 11:39

fix tests

c6506fe

fix linting

2f5f4fe

robertnishihara reviewed Jun 25, 2018

View reviewed changes

fix linting

603821d

fix

c54ac4c

pcmoritz added 4 commits June 26, 2018 00:41

fix python GCS code (?)

788ad35

fix

2b881be

fix

10e3053

put client info into primary context

6d1ab02

pcmoritz added 3 commits June 26, 2018 17:15

fixes

3007103

fix

573defc

fix

3611113

concretevitamin reviewed Jun 28, 2018

View reviewed changes

pcmoritz force-pushed the xray-data-shard branch from 66d6b85 to d4697de Compare June 30, 2018 19:52

concretevitamin added 2 commits June 30, 2018 12:53

With sharding fixed: make primary load credis master, shard load member

5e1a619

Fix indent

2932c4b

pcmoritz force-pushed the xray-data-shard branch from d4697de to 442f65b Compare June 30, 2018 19:57

pcmoritz added 2 commits June 30, 2018 12:58

use xray

442f65b

make less verbose

098d5d9

fix

8b094a7

updates

2c2b226

fix linting

7457a70

robertnishihara approved these changes Jul 1, 2018

View reviewed changes

robertnishihara merged commit 762bdf6 into ray-project:master Jul 1, 2018

robertnishihara deleted the xray-data-shard branch July 1, 2018 01:42

heyucongtom pushed a commit to heyucongtom/ray that referenced this pull request Jul 1, 2018

Implementing Gcs sharding after putting Gcs data into redis data shard (

33a2baa

ray-project#2298)

heyucongtom mentioned this pull request Jul 1, 2018

[WIP] Implementing Gcs sharding after putting Gcs data into redis data shard #2334

Closed

heyucongtom mentioned this pull request Jul 17, 2018

[xray] Implementing Gcs sharding #2409

Merged

[xray] Put GCS data into the redis data shard #2298

[xray] Put GCS data into the redis data shard #2298

Conversation

pcmoritz commented Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 25, 2018

AmplabJenkins commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyucongtom Jun 25, 2018 • edited Loading

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 Jun 25, 2018

AmplabJenkins commented Jun 25, 2018

AmplabJenkins commented Jun 25, 2018

AmplabJenkins commented Jun 26, 2018

AmplabJenkins commented Jun 26, 2018

AmplabJenkins commented Jun 26, 2018

AmplabJenkins commented Jun 26, 2018

AmplabJenkins commented Jun 27, 2018

AmplabJenkins commented Jun 28, 2018

AmplabJenkins commented Jun 28, 2018

AmplabJenkins commented Jun 28, 2018

concretevitamin commented Jun 28, 2018

concretevitamin Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 29, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jun 30, 2018

AmplabJenkins commented Jul 1, 2018

pcmoritz commented Jun 25, 2018 •

edited

Loading

heyucongtom Jun 25, 2018 •

edited

Loading

concretevitamin Jun 28, 2018 •

edited

Loading