[xray] Implementing Gcs sharding #2409

heyucongtom · 2018-07-17T02:07:00Z

What do these changes do?

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.

Related issue number

AmplabJenkins · 2018-07-17T02:28: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/6672/
Test FAILed.

AmplabJenkins · 2018-07-17T03:59:54Z

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

heyucongtom · 2018-07-18T14:55:33Z

@robertnishihara
"bash ../src/plasma/test/run_tests.sh" this test seems to be failing.
Also I need some help to check the sharding scheme for request-notification & subscribeAsync. Still on the way of reading through the codebase.

AmplabJenkins · 2018-07-18T16:02:37Z

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

AmplabJenkins · 2018-07-18T17:19:16Z

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

AmplabJenkins · 2018-07-19T10:52: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/6706/
Test FAILed.

AmplabJenkins · 2018-07-19T12:01: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/6707/
Test PASSed.

AmplabJenkins · 2018-07-19T15:54: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/6709/
Test FAILed.

AmplabJenkins · 2018-07-19T17:16:20Z

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

AmplabJenkins · 2018-07-20T01:47:12Z

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

AmplabJenkins · 2018-07-20T02:47:02Z

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

AmplabJenkins · 2018-07-20T07:01:32Z

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

robertnishihara · 2018-07-20T18:31:16Z

In this PR, you should get rid of this if statement

ray/test/runtest.py

Lines 1157 to 1159 in 2a3b026

    
           if os.environ.get("RAY_USE_XRAY") == "1": 
        
               print("XRAY currently supports only a single Redis shard.") 
        
               kwargs["num_redis_shards"] = 1

so that the sharded case will be tested more.

AmplabJenkins · 2018-07-21T03:48:28Z

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

stephanie-wang · 2018-08-22T20:53:15Z

src/ray/gcs/client.cc

-static void GetRedisShards(redisContext *context, std::vector<std::string> *addresses,
-                           std::vector<int> *ports) {
+static void GetRedisShards(redisContext *context, std::vector<std::string>& addresses,
+                           std::vector<int>& ports) {


Sorry for the nit, but to keep the style consistent, we usually put the & with the variable name, so &addresses and &ports instead of & addresses and & ports.

stephanie-wang

Thanks! Left one small comment, but otherwise looks great :)

heyucongtom · 2018-08-22T21:42:23Z

Thanks! I would wait for the tests to finish and try linting it a little bit.

…

On Wednesday, August 22, 2018, Stephanie Wang ***@***.***> wrote: ***@***.**** approved this pull request. Thanks! Left one small comment, but otherwise looks great :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2409 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGbH8YR2MRn4YCeX1X0TeqwTUB7hGw7vks5uTcTTgaJpZM4VSEPq> .

AmplabJenkins · 2018-08-22T22:02: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/7677/
Test PASSed.

heyucongtom · 2018-08-22T23:09:26Z

@robertnishihara @stephanie-wang There's some error where "GET NumRedisShards" is getting a nil reply after I move the things into the constructor. My guess is that this command is not properly setup when the constructor is called. Please feel free to take a look at it

/home/travis/build/ray-project/ray/src/ray/gcs/client.cc:23 Check failed: num_attempts < RayConfig::instance().redis_db_connect_retries() No entry found for NumRedisShards

AmplabJenkins · 2018-08-25T23:24:49Z

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

AmplabJenkins · 2018-08-26T03:08: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/7769/
Test PASSed.

robertnishihara · 2018-08-26T07:22:50Z

src/ray/gcs/client.cc

+  // Used since changing the API of Connect is breaking some tests.
+  // Would be removed soon in the future.
+  const bool DUMMY_BOOL = true;
+  RAY_CHECK_OK(primary_context_->Connect(address, port, DUMMY_BOOL));


How about we just pass in true instead of defining DUMMY_BOOL. Maybe something like pass in true /* sharding */.

I agree it would be best to just have a default argument, but it's ok to do that separately.

robertnishihara · 2018-08-26T07:24:12Z

src/ray/gcs/client.cc

+    }
+
+    // Populate shard_contexts.
+    for (unsigned int i = 0; i < addresses.size(); ++i) {


We essentially don't use unsigned int anywhere in the codebase. Can you just use int here? This also applies below.

robertnishihara · 2018-08-26T07:25:04Z

src/ray/gcs/client.cc

+
+    // Populate shard_contexts.
+    for (unsigned int i = 0; i < addresses.size(); ++i) {
+      // Slower than emplace but resource safe.


I'm not sure this comment is helpful. This piece of code is definitely not performance critical. Maybe remove the comment.

robertnishihara · 2018-08-26T07:26:23Z

src/ray/gcs/client.cc

+      shard_contexts_.push_back(std::make_shared<RedisContext>());
+    }
+
+    // Call connect for all contexts. Safe to do many times.


It's safe to call Connect multiple times? Why is that?

robertnishihara · 2018-08-26T07:26:49Z

src/ray/gcs/client.cc

+    }
+
+    // Call connect for all contexts. Safe to do many times.
+    // Here shard_contexts_.size() == addresses.size();


Consider making this comment a RAY_CHECK(shard_contexts_.size() == addresses.size()).

AmplabJenkins · 2018-08-29T20:21:23Z

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

AmplabJenkins · 2018-08-29T20:51:57Z

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

AmplabJenkins · 2018-08-29T23:21: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/7867/
Test FAILed.

AmplabJenkins · 2018-08-31T07:55:45Z

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

AmplabJenkins · 2018-08-31T20:17: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/7914/
Test FAILed.

Yucong He and others added 3 commits July 16, 2018 20:06

another commit

d8608dc

Merge branch 'cur_master' into gcs_sharding_2

73bbca1

Fixed gcs_sharding

23fa1cf

heyucongtom changed the title ~~Gcs sharding 2~~ [WIP] Implementing Gcs sharding after putting Gcs data into redis data shard Jul 17, 2018

fix

c0dd5ee

heyucongtom force-pushed the gcs_sharding_2 branch from 515c0ea to c0dd5ee Compare July 18, 2018 16:07

more fixes

0786a59

fix

6b5a765

Yucong He and others added 2 commits July 19, 2018 22:41

lint

2f3ece2

lint lint

482ba00

minor fix

f7bc112

fix

df0a141

Merge branch 'master' into gcs_sharding_2

7ba2e9d

fix tests

fa1eaa8

Yucong He and others added 2 commits July 22, 2018 10:24

Merge branch 'master' into gcs_sharding_2

808a8e8

try fix

2c5a952

Yucong He and others added 5 commits August 16, 2018 17:55

fix

b7ea04c

Merge branch 'master' into gcs_sharding_2

49bb845

Merge branch 'master' into gcs_sharding_2

d332db6

fix fix

30ec166

Fix

718acfc

stephanie-wang reviewed Aug 22, 2018

View reviewed changes

stephanie-wang approved these changes Aug 22, 2018

View reviewed changes

fix test ah

0f30500

fix more tests

369d9c3

robertnishihara reviewed Aug 26, 2018

View reviewed changes

Yucong He and others added 3 commits August 28, 2018 22:14

Merge branch 'master' into gcs_sharding_2

be75798

fix

4064cb1

more fixes

718a442

change int to size_t to avoid warning

3c0fbd7

lint1

51be5ca

final lint

730f39a

robertnishihara approved these changes Aug 31, 2018

View reviewed changes

robertnishihara changed the title ~~[WIP] Implementing Gcs sharding after putting Gcs data into redis data shard~~ Implementing Gcs sharding after putting Gcs data into redis data shard Aug 31, 2018

robertnishihara changed the title ~~Implementing Gcs sharding after putting Gcs data into redis data shard~~ [xray] Implementing Gcs sharding Aug 31, 2018

robertnishihara merged commit 5b45f0b into ray-project:master Aug 31, 2018

[xray] Implementing Gcs sharding #2409

[xray] Implementing Gcs sharding #2409

Uh oh!

Conversation

heyucongtom commented Jul 17, 2018

What do these changes do?

Related issue number

Uh oh!

AmplabJenkins commented Jul 17, 2018

Uh oh!

AmplabJenkins commented Jul 17, 2018

Uh oh!

heyucongtom commented Jul 18, 2018

Uh oh!

AmplabJenkins commented Jul 18, 2018

Uh oh!

AmplabJenkins commented Jul 18, 2018

Uh oh!

AmplabJenkins commented Jul 19, 2018

Uh oh!

AmplabJenkins commented Jul 19, 2018

Uh oh!

AmplabJenkins commented Jul 19, 2018

Uh oh!

AmplabJenkins commented Jul 19, 2018

Uh oh!

AmplabJenkins commented Jul 20, 2018

Uh oh!

AmplabJenkins commented Jul 20, 2018

Uh oh!

AmplabJenkins commented Jul 20, 2018

Uh oh!

robertnishihara commented Jul 20, 2018

Uh oh!

AmplabJenkins commented Jul 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

heyucongtom commented Aug 22, 2018 via email

Uh oh!

AmplabJenkins commented Aug 22, 2018

Uh oh!

heyucongtom commented Aug 22, 2018

Uh oh!

AmplabJenkins commented Aug 25, 2018

Uh oh!

AmplabJenkins commented Aug 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Aug 29, 2018

Uh oh!

AmplabJenkins commented Aug 29, 2018

Uh oh!

AmplabJenkins commented Aug 29, 2018

Uh oh!

AmplabJenkins commented Aug 31, 2018

Uh oh!

AmplabJenkins commented Aug 31, 2018

Uh oh!

Uh oh!