Request and cancel notifications in the new GCS API #1758

stephanie-wang · 2018-03-20T22:30:41Z

What do these changes do?

This allows a caller to request and cancel notifications about a specific key in a GCS table.

AmplabJenkins · 2018-03-20T23:03: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/4445/
Test FAILed.

AmplabJenkins · 2018-03-21T00:54: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/4447/
Test PASSed.

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

…odules

…s instead of through publish

concretevitamin

A few concerns:

I suggest we stay away from boost. Asio, sure, but not other stuff.
tables.h has become larger and contains many definitions instead of pure declaration. Consider separating into a tables.cc?

concretevitamin · 2018-03-21T04:25:16Z

src/common/redis_module/ray_redis_module.cc

+  RAY_CHECK(RedisModule_StringToLongLong(
+                pubsub_channel_str, &pubsub_channel_long) == REDISMODULE_OK)
+      << "Prefix must be a valid TablePrefix";
+  char pubsub_channel[sizeof(TablePubsub) + 1];


For the null terminator, although this doesn't measure the maximum number of characters needed properly...I changed this to TablePubsub_MAX.

concretevitamin · 2018-03-21T04:29:02Z

src/common/redis_module/ray_redis_module.cc

@@ -537,6 +617,75 @@ int TableLookup_RedisCommand(RedisModuleCtx *ctx,
  return REDISMODULE_OK;
 }

+int TableRequestNotifications_RedisCommand(RedisModuleCtx *ctx,


Please add some doc

concretevitamin · 2018-03-21T04:29:20Z

src/common/redis_module/ray_redis_module.cc

+  }
+}
+
+int TableCancelNotifications_RedisCommand(RedisModuleCtx *ctx,


concretevitamin · 2018-03-21T04:30:18Z

src/common/redis_module/ray_redis_module.cc

@@ -554,14 +703,15 @@ bool is_nil(const std::string &data) {
 int TableTestAndUpdate_RedisCommand(RedisModuleCtx *ctx,
                                    RedisModuleString **argv,
                                    int argc) {
-  if (argc != 4) {
+  if (argc != 5) {


Let's add doc on args:

// This is a temporary redis command that will be removed once // the GCS uses https://github.com/pcmoritz/credis. // Be careful, this only supports Task Table payloads. // Args: // prefix_str: ... // ...

concretevitamin · 2018-03-21T04:32:33Z

src/common/redis_module/ray_redis_module.cc

+            RedisModule_Call(ctx, "PUBLISH", "ss", client_channel, data);
+        if (reply == NULL) {
+          RedisModule_CloseKey(notification_key);
+          RedisModule_ReplyWithError(ctx, "error during PUBLISH");


Should this be return RedisModule_ReplyWithError(ctx, "error during PUBLISH");? Otherwise the key will be closed twice (which I think is fine, just somewhat stands out as odd).

concretevitamin · 2018-03-21T04:33:27Z

src/common/redis_module/ray_redis_module.cc

+        }
+      }
+    }
+    RedisModule_CloseKey(notification_key);


This redis command needs to return an int (OK or ERROR).

It does (further down).

concretevitamin · 2018-03-21T04:33:37Z

src/common/redis_module/ray_redis_module.cc

@@ -517,13 +596,14 @@ int TableAdd_RedisCommand(RedisModuleCtx *ctx,
 int TableLookup_RedisCommand(RedisModuleCtx *ctx,
                             RedisModuleString **argv,
                             int argc) {
-  if (argc != 3) {
+  if (argc != 4) {


Please document args

concretevitamin · 2018-03-21T04:34:24Z

src/common/redis_module/ray_redis_module.cc

@@ -394,17 +447,18 @@ bool PublishObjectNotification(RedisModuleCtx *ctx,
 int TableAdd_RedisCommand(RedisModuleCtx *ctx,


This command seems very bloated now. A rule of thumb I learned was any func > 50 lines should probably be broken down.

Sounds good, although a lot of the bloat will hopefully go away soon, since a lot of the code is here is only necessary to support legacy code (like the task table).

concretevitamin · 2018-03-21T04:40:42Z

src/ray/gcs/redis_context.cc

@@ -24,24 +24,37 @@ void GlobalRedisCallback(void *c, void *r, void *privdata) {
  }
  int64_t callback_index = reinterpret_cast<int64_t>(privdata);
  redisReply *reply = reinterpret_cast<redisReply *>(r);
+  // We use an optional response to distinguish between a nil response and a
+  // response of an empty string.
+  boost::optional<const std::string &> optional_data;


Please don't use boost optional. My unpopular opinion is we shouldn't use boost at all.

boost::optional is not in the approved subset by Google style: http://google.github.io/styleguide/cppguide.html#Boost

We should transition to abseil. https://abseil.io/tips/123

+1 to not using boost anywhere except for asio.

stephanie-wang · 2018-03-21T05:40:01Z

I actually just updated this to return a std::vector instead, since it works better with the planned changes to support a set of entries in a key. Can you take a look again?
Sounds good.

AmplabJenkins · 2018-03-21T05:50:29Z

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

AmplabJenkins · 2018-03-21T06:40: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/4451/
Test PASSed.

AmplabJenkins · 2018-03-21T07:16:11Z

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

AmplabJenkins · 2018-03-21T19:07: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/4455/
Test PASSed.

concretevitamin · 2018-03-21T19:14:24Z

Is this ready for another look?

…

On Wed, Mar 21, 2018 at 12:07 UCB AMPLab ***@***.***> wrote: Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/4455/ Test PASSed. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1758 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkLHijL3_-I2rgCk_eh-MD1yOym62TGks5tgqT8gaJpZM4SywRm> .

stephanie-wang · 2018-03-21T19:20:12Z

Yes, that would be great, thanks, @concretevitamin! I made some significant changes to the gcs/tables.h API, so it would be good to get your opinion on that as well.

elibol · 2018-03-21T19:42:36Z

LGTM. Just need to make sure we coordinate this change with the soon-to-be xray-task-forwarding PR, which uses the object table lookup failure callback.

concretevitamin · 2018-03-21T20:26:50Z

Taking a look!

…

On Wed, Mar 21, 2018 at 12:42 PM Melih Elibol ***@***.***> wrote: LGTM. Just need to make sure we coordinate this change with the soon-to-be xray-task-forwarding PR, which uses the object table lookup failure callback. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1758 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkLHuKyasC8bbNYj_MzdEN8xnBLS7f7ks5tgq0ygaJpZM4SywRm> .

concretevitamin · 2018-03-21T19:45:37Z

src/common/redis_module/ray_redis_module.cc

@@ -53,6 +53,31 @@ static const char *table_prefixes[] = {
    NULL, "TASK:", "CLIENT:", "OBJECT:", "FUNCTION:",
 };

+/// Parse a Redis string into a TablePubsub channel.
+TablePubsub ParseTablePubsub(RedisModuleString *pubsub_channel_str) {


const (to indicate no changes/freeing, etc)

concretevitamin · 2018-03-21T19:46:06Z

src/common/redis_module/ray_redis_module.cc

+
+/// Format a pubsub channel for a specific key. pubsub_channel_str should
+/// contain a valid TablePubsub.
+RedisModuleString *FormatPubsubChannel(RedisModuleCtx *ctx,


const the args if you can

concretevitamin · 2018-03-21T19:48:20Z

src/ray/gcs/tables.h

@@ -158,6 +131,9 @@ class Table {
  TablePubsub pubsub_channel_;
  /// The prefix to use for keys in this table.
  TablePrefix prefix_;
+  /// Whether we have subscribed to the table yet. Each client may only


"Whether" is not informative, I actually had to search for this arg to understand.

Say something like, ">= 0 iff we have subscribed, otherwise -1."

concretevitamin · 2018-03-21T19:53:37Z

src/ray/gcs/tables.cc

+  flatbuffers::FlatBufferBuilder fbb;
+  fbb.ForceDefaults(true);
+  fbb.Finish(Data::Pack(fbb, data.get()));
+  RAY_RETURN_NOT_OK(context_->RunAsync("RAY.TABLE_ADD", id, fbb.GetBufferPointer(),


Can be simplified to return context_->RunAsync(..);

concretevitamin · 2018-03-21T19:53:52Z

src/ray/gcs/tables.cc

+        return true;
+      });
+  std::vector<uint8_t> nil;
+  RAY_RETURN_NOT_OK(context_->RunAsync("RAY.TABLE_LOOKUP", id, nil.data(), nil.size(),


can be simplified

concretevitamin · 2018-03-21T20:05:31Z

src/ray/gcs/redis_context.cc

    RAY_LOG(FATAL) << "Fatal redis error of type " << reply->type << " and with string "
                   << reply->str;
  }
+
+  if (callback_index >= 0) {


Warrants an anon-namespace helper since it repeats, in my opinion

MaybeRemoveCallback(callback_index);

for

+ if (callback_index >= 0) { + bool delete_callback = RedisCallbackManager::instance().get(callback_index)(data); + if (delete_callback) { + RedisCallbackManager::instance().remove(callback_index); + } + }

concretevitamin · 2018-03-21T20:08:55Z

src/ray/gcs/redis_context.cc

-    data = std::string(reply->str, reply->len);
-  } else if (reply->type == REDIS_REPLY_STATUS) {
-  } else if (reply->type == REDIS_REPLY_ERROR) {
+  std::vector<std::string> data;


It seems data always has size 1 throughout this PR. If that's true, what's the reason of changing the iface to vector (by default I think it allocates a few more slots than the absolute minimum, which is 1; this might be wrong though)?

Eventually we want to transition to an interface where an operation appends a value instead of just overwriting it, which isn't flexible enough to support the object table, for instance. One thing that could work is if we use the std::vector interface internally, but the interface for the table caller can still take in just a single data. Would you prefer that we do that in this PR?

concretevitamin · 2018-03-21T20:53:36Z

src/ray/gcs/client_test.cc

-  ASSERT_EQ(data->managers, std::vector<std::string>({"A", "B"}));
+                 const std::vector<ObjectTableDataT> &data) {
+  ASSERT_EQ(data.size(), 1);
+  ASSERT_EQ(data[0].managers, std::vector<std::string>({"A", "B"}));


I imagine (and hope) ASSERT_EQ(data[0].managers, {"A", "B"}); just works.

It does not, I think because of the ASSERT_EQ macro:

error: macro "ASSERT_EQ" passed 3 arguments, but takes just 2 ASSERT_EQ(data[0].managers, {"A", "B"});

concretevitamin · 2018-03-21T20:53:45Z

src/ray/gcs/client_test.cc

  // Check that the object entry was added.
-  ASSERT_EQ(data->managers, std::vector<std::string>({"A", "B"}));
+  ASSERT_EQ(data.size(), 1);
+  ASSERT_EQ(data[0].managers, std::vector<std::string>({"A", "B"}));


concretevitamin · 2018-03-21T20:54:21Z

src/ray/gcs/client_test.cc

+                                  const std::vector<ObjectTableDataT> &data) {
+    ASSERT_EQ(data.size(), 1);
+    // Check that the object entry was added.
+    ASSERT_EQ(data[0].managers, std::vector<std::string>({"A", "B"}));


concretevitamin · 2018-03-21T21:24:00Z

For ASSERT_EQ I think a convention is to put the expected literals as first arg. Would that fix the compiler issue?

…

On Wed, Mar 21, 2018 at 14:22 Stephanie Wang ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ray/gcs/redis_context.cc <#1758 (comment)>: > @@ -24,24 +24,31 @@ void GlobalRedisCallback(void *c, void *r, void *privdata) { } int64_t callback_index = reinterpret_cast<int64_t>(privdata); redisReply *reply = reinterpret_cast<redisReply *>(r); - std::string data = ""; - if (reply->type == REDIS_REPLY_NIL) { - // Respond with blank string, which triggers a failure callback for lookups. - } else if (reply->type == REDIS_REPLY_STRING) { - data = std::string(reply->str, reply->len); - } else if (reply->type == REDIS_REPLY_ARRAY) { - reply = reply->element[reply->elements - 1]; - data = std::string(reply->str, reply->len); - } else if (reply->type == REDIS_REPLY_STATUS) { - } else if (reply->type == REDIS_REPLY_ERROR) { + std::vector<std::string> data; Eventually we want to transition to an interface where an operation appends a value instead of just overwriting it, which isn't flexible enough to support the object table, for instance. One thing that could work is if we use the std::vector interface internally, but the interface for the table caller can still take in just a single data. Would you prefer that we do that in this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1758 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkLHjZhnzf_NZP7VZZwBToS6xRzEmstks5tgsR2gaJpZM4SywRm> .

stephanie-wang · 2018-03-21T21:25:12Z

It does not fix the compiler issue.

This reverts commit 6e3e690.

…kes a reference to a single item

AmplabJenkins · 2018-03-21T21:55: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/4456/
Test FAILed.

AmplabJenkins · 2018-03-21T22:41: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/4457/
Test PASSed.

AmplabJenkins · 2018-03-22T00:35:18Z

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

concretevitamin

LGTM. Feel free to self-merge after addressing a few nits!

concretevitamin · 2018-03-22T02:05:22Z

src/ray/gcs/tables.cc

+  std::vector<uint8_t> nil;
+  return context_->RunAsync("RAY.TABLE_LOOKUP", id, nil.data(), nil.size(), prefix_,
+                            pubsub_channel_, callback_index);
+  return Status::OK();


How does double returning pass all our tests?

I don't know...oh dear.

concretevitamin · 2018-03-22T02:05:59Z

src/ray/gcs/tables.cc

+  fbb.Finish(Data::Pack(fbb, data.get()));
+  return context_->RunAsync("RAY.TABLE_ADD", id, fbb.GetBufferPointer(), fbb.GetSize(),
+                            prefix_, pubsub_channel_, callback_index);
+  return Status::OK();


double returning ?!

concretevitamin · 2018-03-22T02:07:02Z

src/ray/gcs/redis_context.cc

+    }
+  }
+}
+}


nit: leave a blank line to be symmetric

Hmm, lint actually complains about this.

concretevitamin · 2018-03-22T02:08:27Z

src/ray/gcs/client_test.cc

@@ -42,7 +42,12 @@ class TestGcs : public ::testing::Test {

  virtual void Stop() = 0;

+  int64_t NumCallbacks() { return num_callbacks_; }


nit: int64_t num_callbacks() const { return num_callbacks_; }

smaller case for cheap methods; const when you can

up to you

concretevitamin · 2018-03-22T02:09:28Z

src/common/redis_module/ray_redis_module.cc

@@ -1429,6 +1642,18 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx,
    return REDISMODULE_ERR;
  }

+  if (RedisModule_CreateCommand(ctx, "ray.table_request_notifications",


Side note: I probably won't be able to port all of these to credis myself...

Yeah, I can help with that. I hope though, that this interface will actually be easier to implement in credis than trying to support everything in vanilla Ray's Redis modules.

AmplabJenkins · 2018-03-22T04:45:00Z

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

concretevitamin · 2018-03-22T05:51:44Z

New changes LGTM

AmplabJenkins · 2018-03-22T06:25: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/4468/
Test PASSed.

* commit 'f69cbd35d4e86f2a3c2ace875aaf8166edb69f5d': (64 commits) Bump version to 0.4.0. (ray-project#1745) Fix monitor.py bottleneck by removing excess Redis queries. (ray-project#1786) Convert the ObjectTable implementation to a Log (ray-project#1779) Acquire worker lock when importing actor. (ray-project#1783) Introduce a log interface for the new GCS (ray-project#1771) [tune] Fix linting error (ray-project#1777) [tune] Added pbt with keras on cifar10 dataset example (ray-project#1729) Add a GCS table for the xray task flatbuffer (ray-project#1775) [tune] Change tune resource request syntax to be less confusing (ray-project#1764) Remove from X import Y convention in RLlib ES. (ray-project#1774) Check if the provider is external before getting the config. (ray-project#1743) Request and cancel notifications in the new GCS API (ray-project#1758) Fix resource bookkeeping for blocked actor methods. (ray-project#1766) Fix bug when connecting another driver in local case. (ray-project#1760) Define string prefixes for all tables in the new GCS API (ray-project#1755) [rllib] Update RLlib to work with new actor scheduling behavior (ray-project#1754) Redirect output of all processes by default. (ray-project#1752) Add API for getting total cluster resources. (ray-project#1736) Always send actor creation tasks to the global scheduler. (ray-project#1757) Print error when actor takes too long to start, and refactor error me… (ray-project#1747) ... # Conflicts: # python/ray/rllib/__init__.py # python/ray/rllib/dqn/dqn.py # python/ray/rllib/dqn/dqn_evaluator.py # python/ray/rllib/dqn/dqn_replay_evaluator.py # python/ray/rllib/optimizers/__init__.py # python/ray/rllib/tuned_examples/pong-dqn.yaml

stephanie-wang changed the title ~~Request and cancel notifications in the new GCS API~~ [WIP] Request and cancel notifications in the new GCS API Mar 20, 2018

stephanie-wang requested review from concretevitamin and pcmoritz March 20, 2018 22:31

stephanie-wang added 9 commits March 20, 2018 20:33

Add TableRequestNotifications and TableCancelNotifications to Redis m…

76237ba

…odules

Add RequestNotifications and CancelNotifications to generic GCS Table

2f90177

Add tests for subscribing to specific keys

80e3440

Remove TODO!

b81ad88

Return the current value at the key directly from RequestNotification…

e6b68ff

…s instead of through publish

Add unit test for Lookup failure callback

916dfe3

Modify tests to account for empty subscription response

9ae2372

Remove ObjectTable notification methods

a7b9db1

Clean up message parsing and doc in redis context

a3651a4

concretevitamin suggested changes Mar 21, 2018

View reviewed changes

stephanie-wang added 2 commits March 20, 2018 22:19

Use vectors of DataT in all GCS callbacks

ce6ed97

Clean up SubscriptionCallback

f37160d

stephanie-wang force-pushed the gcs-request-notifications branch from 21e2468 to f37160d Compare March 21, 2018 05:24

Move Table definitions into tables.cc

dd1e64d

Refactor and document redis modules

a787d16

stephanie-wang added 2 commits March 21, 2018 11:00

doc

a8a5e5f

Fix new GCS build

6e3e690

stephanie-wang changed the title ~~[WIP] Request and cancel notifications in the new GCS API~~ Request and cancel notifications in the new GCS API Mar 21, 2018

concretevitamin reviewed Mar 21, 2018

View reviewed changes

Cleanups

23aa4a2

stephanie-wang added 4 commits March 21, 2018 14:43

Revert "Fix new GCS build"

de54e58

This reverts commit 6e3e690.

Use vectors for internal callback interface, user-facing interface ta…

1add01c

…kes a reference to a single item

Fix new GCS build

35c607f

Add unit test for Lookup failure callback

5ff6962

Fix compiler errors

8ce7d1a

concretevitamin approved these changes Mar 22, 2018

View reviewed changes

Cleanup

b8f80f6

stephanie-wang added 2 commits March 21, 2018 22:33

Publish the entry ID with the notification

c2f4732

Check that the ID for a notification matches in client tests

57e0653

stephanie-wang merged commit 8704c86 into ray-project:master Mar 22, 2018

stephanie-wang deleted the gcs-request-notifications branch March 22, 2018 17:31

		@@ -394,17 +447,18 @@ bool PublishObjectNotification(RedisModuleCtx *ctx,
		int TableAdd_RedisCommand(RedisModuleCtx *ctx,

		@@ -42,7 +42,12 @@ class TestGcs : public ::testing::Test {

		virtual void Stop() = 0;

		int64_t NumCallbacks() { return num_callbacks_; }

Request and cancel notifications in the new GCS API #1758

Request and cancel notifications in the new GCS API #1758

Conversation

stephanie-wang commented Mar 20, 2018

What do these changes do?

AmplabJenkins commented Mar 20, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

concretevitamin 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

stephanie-wang commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

concretevitamin commented Mar 21, 2018 via email

stephanie-wang commented Mar 21, 2018

elibol commented Mar 21, 2018

concretevitamin commented Mar 21, 2018 via email

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

concretevitamin commented Mar 21, 2018 via email

stephanie-wang commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 21, 2018

AmplabJenkins commented Mar 22, 2018

concretevitamin 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

AmplabJenkins commented Mar 22, 2018

concretevitamin commented Mar 22, 2018

AmplabJenkins commented Mar 22, 2018