Introduce set data structure in GCS #4199

kfstorm · 2019-02-28T10:55:17Z

What do these changes do?

Some GCS tables keep tracking evicted entries and never release the memory even if they are not used anyomore. For example, object table tracks the location history of one object. It may contain multiple entries for a single client because the object has multiple addition/eviciton history on this client.

Although we recently enabled redis LRU cache for GCS (#3499), I believe the set structure is the right way to automatically recycle GCS memory, for most GCS table types. This pull request is the beginning of changing most GCS tables from Log-based to Set-based implementation. It just changed object table to Set-based. We can change other tables to Set-based after it's proved useful.

Currently the Log class in tables.h only allows one write operation: Append (backed by redis RAY.TABLE_APPEND command). I added a new class Set with Add and Remove operations (backed by redis RAY.SET_ADD and RAY.SET_REMOVE commands). This pull request only touched minimum code of Log and Table class, to keep other GCS tables unaffected.

Related issue number

#954

…nce.

AmplabJenkins · 2019-02-28T14:00: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/12389/
Test FAILed.

AmplabJenkins · 2019-03-01T09:20:19Z

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

AmplabJenkins · 2019-03-02T18:06: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/12473/
Test FAILed.

stephanie-wang

Can you also add a couple tests for the failure scenarios (e.g., removing from a set that is already empty) to this test? I guess there's a bit of duplication here, but it's easier to test in Python.

Thanks a lot! I'm excited for this change :)

stephanie-wang · 2019-03-06T06:24:57Z

src/ray/raylet/node_manager.cc

            const std::unordered_set<ray::ClientID> &clients, bool has_been_created) {
+          // TODO(raulchen): LookupLocations will set has_been_created to false if
+          // the object does not exist on any nodes due to eviction
+          has_been_created = true;


Hmm this is a bit ugly. What if we kept the key in Redis even for objects that have been evicted from all nodes? Then, we could at least match the current behavior.

Another possibility (which I prefer) is to not rely on the has_been_created flag at all, and maybe just put an exception if: (a) LookupLocations returns empty, and (b) the object is not local.

I prefer to not reply on has_been_created as well.

I thought of a case that depending on it would have problems. Let's say an actor finishes a task and saves the result to object store, then it does a checkpoint. Then the node dies before object info is saved to GCS. And the actor gets reconstructed on another node and resumes from the checkpoint. In this case, we cannot re-execute the task and the object would appear like never have been created.

Removed has_been_created from some callbacks and parameter of UpdateObjectLocations.

stephanie-wang · 2019-03-06T06:30:30Z

src/ray/object_manager/object_directory.cc

+  if (!location_updates.empty()) {
    // If there are entries, then the object has been created. Once this flag
    // is set to true, it should never go back to false.
    *has_been_created = true;


It seems like the has_been_created flag isn't really necessary anymore, since the only place we were really using it was for evicted actor objects. We are using it here, though, to make sure that we received a first subscription notification before using the cached object locations.

How about we rename has_been_created to subscribed, and just set it to true as soon as we receive the first notification callback for an object? We should also remove it from the ObjectDirectory public API.

I agree with renaming has_been_created to subscribed, and I tried. But I found another usage of has_been_created here. I don't know how should I update it.

Hmm if you follow that flag up to the node manager, it looks like it's not actually used. Sorry, I think this was my bad :)

I think it's okay to just remove the usage in the ReconstructionPolicy as well.

stephanie-wang · 2019-03-06T06:30:50Z

src/ray/object_manager/object_directory.cc

    *has_been_created = true;
  }
-  for (const auto &object_table_data : location_history) {
+  if (mode == GcsTableNotificationMode::CURRENT_VALUE) {


Can you add a comment explaining why we should clear client_ids? This wasn't immediately obvious to me.

If the caller invokes UpdateObjectLocations with CURRENT_VALUE and a non-empty client_ids, and client_ids has more elements than location_updates, this means some clients were removed (during redis reconnection process). Anyway, I'll remove CURRENT_VALUE for simplicity.

src/ray/gcs/format/gcs.fbs

src/ray/gcs/tables.h

src/ray/gcs/client_test.cc

AmplabJenkins · 2019-03-06T13:15: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/12601/
Test FAILed.

AmplabJenkins · 2019-03-06T17:00:06Z

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

stephanie-wang · 2019-03-06T19:09:53Z

src/ray/gcs/client_test.cc

+        for (int k = 0; k < 3; k++) {
+          // Remove the same entry several times.
+          // Expect no notification if the entry doesn't exist.
+          RAY_CHECK_OK(client->object_table().Remove(job_id, object_ids[i], data, nullptr));


I was under the impression that ObjectTable::Remove would be fatal if the object ID to remove doesn't exist, since we check for Redis errors here. Is that not the case?

In our internal codebase, return OK for removing a non-existent entry is necessary because we need to make sure the operation is idempotent. GCS operations must be idempotent because we deal with redis reconnection and failed commands may be resent after redis failover.

However, return OK for removing non-existent entry can be buggy because the caller may try to remove an entry with slightly different field values. (it's usual in current codebase because we have a special field to mark its write mode, so we often only fill key fields such as ids if we want to append an entry with deletion mode) This results in unexpected behavior and hard to find the bug.

I'll change RAY.SET_REMOVE to return an error if the entry doesn't exist. And keep RAY.SET_ADD to return OK if the entry already exists. Sounds OK?

stephanie-wang · 2019-03-06T19:10:47Z

src/ray/gcs/redis_module/ray_redis_module.cc

 }

-int Set_DoWrite(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, bool is_add) {
+int Set_DoWrite(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, bool is_add, bool &changed) {


The convention we've been using is to use pointers instead of references for function outputs.

Just learnt that google style also requires using references for output args, https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Thanks! Updated to bool *

stephanie-wang · 2019-03-06T19:12:54Z

src/ray/gcs/tables.h

                   const NotificationCallback &subscribe,
                   const SubscriptionCallback &done) {
-    return Log<ID, Data>::SubscribeWithNotificationMode(job_id, client_id, subscribe,
+    return Log<ID, Data>::Subscribe(job_id, client_id, subscribe,


I guess we could also do using Log<...>::Subscribe here.

For Set, I only want to expose the overloaded Subscribe function with the NotificationCallback parameter. If we use using Log<...>::Subscribe here, both overloaded functions are exposed.

stephanie-wang · 2019-03-06T19:14:11Z

src/ray/object_manager/object_directory.cc

+  if (!location_updates.empty()) {
    // If there are entries, then the object has been created. Once this flag
    // is set to true, it should never go back to false.
    *has_been_created = true;


Hmm if you follow that flag up to the node manager, it looks like it's not actually used. Sorry, I think this was my bad :)

I think it's okay to just remove the usage in the ReconstructionPolicy as well.

raulchen · 2019-03-07T02:11:48Z

src/ray/gcs/format/gcs.fbs

+}
+
 table GcsTableEntry {
+  mode: GcsTableNotificationMode;


maybe rename mode to notification_mode, whose meaning is more clear.

Thanks! Updated.

raulchen · 2019-03-07T02:19:43Z

src/ray/gcs/tables.cc

 }

 template class Log<ObjectID, ObjectTableData>;
+template class Set<ObjectID, ObjectTableData>;


template class Log<ObjectID, ObjectTableData>; isn't needed any more, right?

If I remove this line of code, I'll get the following build error:

Undefined symbols for architecture x86_64: "ray::gcs::Log<ray::UniqueID, ObjectTableData>::Delete(ray::UniqueID const&, std::__1::vector<ray::UniqueID, std::__1::allocator<ray::UniqueID> > const&)", referenced from: ray::gcs::TestDeleteKeysFromSet(ray::UniqueID const&, std::__1::shared_ptr<ray::gcs::AsyncGcsClient>, std::__1::vector<std::__1::shared_ptr<ObjectTableDataT>, std::__1::allocator<std::__1::shared_ptr<ObjectTableDataT> > >&) in client_test.cc.o "ray::gcs::Log<ray::UniqueID, ObjectTableData>::Delete(ray::UniqueID const&, ray::UniqueID const&)", referenced from: ray::gcs::TestDeleteKeysFromSet(ray::UniqueID const&, std::__1::shared_ptr<ray::gcs::AsyncGcsClient>, std::__1::vector<std::__1::shared_ptr<ObjectTableDataT>, std::__1::allocator<std::__1::shared_ptr<ObjectTableDataT> > >&) in client_test.cc.o "ray::gcs::Log<ray::UniqueID, ObjectTableData>::Lookup(ray::UniqueID const&, ray::UniqueID const&, std::__1::function<void (ray::gcs::AsyncGcsClient*, ray::UniqueID const&, std::__1::vector<ObjectTableDataT, std::__1::allocator<ObjectTableDataT> > const&)> const&)", referenced from: ray::gcs::TestSet(ray::UniqueID const&, std::__1::shared_ptr<ray::gcs::AsyncGcsClient>) in client_test.cc.o ray::gcs::TestDeleteKeysFromSet(ray::UniqueID const&, std::__1::shared_ptr<ray::gcs::AsyncGcsClient>, std::__1::vector<std::__1::shared_ptr<ObjectTableDataT>, std::__1::allocator<std::__1::shared_ptr<ObjectTableDataT> > >&) in client_test.cc.o ld: symbol(s) not found for architecture x86_64

Ah yeah, I think this is just because Set inherits from Log. :(

raulchen · 2019-03-07T02:26:35Z

src/ray/gcs/tables.h

+template <typename ID, typename Data>
+class Set : private Log<ID, Data>,
+            public SetInterface<ID, Data>,
+            virtual public PubsubInterface<ID> {


It looks a bit weird to me that Set inherits Log, so is Table. It feels more natural to have an abstract super class, say Collection, and have Set, Log and Table inherit it.

But It's fine to do that later.

@stephanie-wang any thoughts?

Yes, that sounds like a good idea! Agreed that we should do it in a future PR.

AmplabJenkins · 2019-03-07T10:17:46Z

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

AmplabJenkins · 2019-03-07T12:00:22Z

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

AmplabJenkins · 2019-03-08T04:10:56Z

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

stephanie-wang

Thanks, this looks great!

stephanie-wang · 2019-03-10T04:56:56Z

Looks like there are some build and lint errors on Travis we should address first.

guoyuhong

Sorry for the late review. I left a few comments.

guoyuhong · 2019-03-10T16:10:25Z

src/ray/gcs/client_test.cc

-                           std::shared_ptr<gcs::AsyncGcsClient> client,
-                           std::vector<std::shared_ptr<ObjectTableDataT>> &data_vector) {
+void TestSet(const JobID &job_id, std::shared_ptr<gcs::AsyncGcsClient> client) {
+  // Append some entries to the log at an object ID.


Since the structure is changed to Set, this comment should be changed correspondingly. Append -> Add, Log -> Set.

guoyuhong · 2019-03-10T16:24:02Z

src/ray/gcs/redis_module/ray_redis_module.cc

+
+  RedisModuleString *key_string = PrefixedKeyString(ctx, prefix_str, id);
+  RedisModuleCallReply *reply =
+      RedisModule_Call(ctx, is_add ? "SADD" : "SREM", "ss", key_string, data);


RedisModule_Call seems slower than the direct c++ interface. Is that because there is no such interface ready for SREM? If that is the case, please add a TODO comment to replace this command to C++ interface if hredis is updated..

Yes, set type API is not available yet. See https://redis.io/topics/modules-intro

guoyuhong · 2019-03-10T16:26:37Z

src/ray/gcs/redis_module/ray_redis_module.cc

    return REDISMODULE_ERR;
  }

+  if (RedisModule_CreateCommand(ctx, "ray.set_add", SetAdd_RedisCommand, "write", 0, 0,


Should be "write pubsub", right?

Seems you are right. But why RAY.TABLE_APPEND is write?

According to https://redis.io/topics/modules-api-ref , RAY.TABLE_APPEND should be also set to "write pubsub". However, it looks like these commands work fine without pubsub... The document does not explain how it will use strflags, but we should keep it right.

I've updated RAY.TABLE_APPEND, RAY.SET_ADD and RAY.SET_REMOVE to write pubsub flag.

guoyuhong · 2019-03-10T16:26:56Z

src/ray/gcs/redis_module/ray_redis_module.cc

+    return REDISMODULE_ERR;
+  }
+
+  if (RedisModule_CreateCommand(ctx, "ray.set_remove", SetRemove_RedisCommand, "write", 0,


The same, should be "write pubsub", right?

guoyuhong · 2019-03-10T16:28:38Z

src/ray/gcs/tables.cc

 Status Log<ID, Data>::Subscribe(const JobID &job_id, const ClientID &client_id,
                                const Callback &subscribe,
                                const SubscriptionCallback &done) {
+  auto subscribeWrapper = [subscribe](AsyncGcsClient *client, const ID &id,


subscribeWrapper seems not consistent with the code style. May be subscribe_wrapper?

guoyuhong

+1. Let's wait for the CI.

AmplabJenkins · 2019-03-11T07:09:44Z

Can one of the admins verify this patch?

AmplabJenkins · 2019-03-11T09:34: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/12745/
Test FAILed.

guoyuhong · 2019-03-11T14:31:15Z

@stephanie-wang It's ready to merge now.

Introduce set data structure in GCS. Change object table to Set insta…

c3d5f67

…nce.

kfstorm added 2 commits March 1, 2019 15:25

Fix a logic bug. Update python code.

4316c9a

lint

9cbbb91

lint again

6edd6ba

raulchen self-requested a review March 3, 2019 04:03

guoyuhong requested review from robertnishihara and stephanie-wang March 4, 2019 03:21

Merge remote-tracking branch 'origin/master' into gcs_set

9174129

guoyuhong requested a review from ericl March 4, 2019 03:22

stephanie-wang requested changes Mar 6, 2019

View reviewed changes

robertnishihara assigned stephanie-wang Mar 6, 2019

kfstorm added 2 commits March 6, 2019 16:45

Remove CURRENT_VALUE mode

234ceb1

Remove 'CURRENT_VALUE'

e1a18cf

Add more test cases

70d2bd6

stephanie-wang reviewed Mar 6, 2019

View reviewed changes

raulchen reviewed Mar 7, 2019

View reviewed changes

kfstorm added 6 commits March 7, 2019 12:10

Merge remote-tracking branch 'origin/master' into gcs_set

8ad87a1

rename has_been_created to subscribed.

a51d8aa

Make changed parameter type of bool *

15e818b

Rename mode to notification_mode

ba0b429

fix build

e22e051

RAY.SET_REMOVE return error if entry doesn't exist

a12eb13

lint

4345a11

Merge remote-tracking branch 'community/master' into gcs_set

8f2fcb1

stephanie-wang approved these changes Mar 10, 2019

View reviewed changes

guoyuhong requested changes Mar 10, 2019

View reviewed changes

kfstorm added 2 commits March 11, 2019 13:02

Address comments

3dff1f5

lint and fix build

184878f

guoyuhong approved these changes Mar 11, 2019

View reviewed changes

stephanie-wang merged commit 7ff56ce into ray-project:master Mar 11, 2019

guoyuhong deleted the gcs_set branch March 12, 2019 03:04

robertnishihara mentioned this pull request Mar 19, 2019

Redis LRU plus object store LRU can lead to fatal crash. #4406

Closed

kfstorm mentioned this pull request Mar 20, 2019

Redis returns OK when removing a non-existent set entry #4434

Merged

Introduce set data structure in GCS #4199

Introduce set data structure in GCS #4199

Uh oh!

Conversation

kfstorm commented Feb 28, 2019

What do these changes do?

Related issue number

Uh oh!

AmplabJenkins commented Feb 28, 2019

Uh oh!

AmplabJenkins commented Mar 1, 2019

Uh oh!

AmplabJenkins commented Mar 2, 2019

Uh oh!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulchen Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AmplabJenkins commented Mar 6, 2019

Uh oh!

AmplabJenkins commented Mar 6, 2019

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!

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 Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 7, 2019

Uh oh!

AmplabJenkins commented Mar 8, 2019

raulchen Mar 6, 2019 •

edited

Loading