Fix LRU eviction of client notification datastructure #4021

pcmoritz · 2019-02-11T23:25:07Z

This should fix #3997

One question that is still open is if this datastructure is cleaned up properly (both if the wait exits and also if clients die that are blocked in a wait).

ericl · 2019-02-11T23:45:41Z

src/ray/gcs/redis_module/ray_redis_module.cc

@@ -39,6 +39,9 @@ extern RedisChainModule module;
    }                                                            \
  }

+/// Map from pub sub channel to clients that are waiting on that channel.
+std::unordered_map<std::string, std::vector<std::string>> notification_map;


Can we add the size of this to the debug string somehow?

We could alternatively print a warning if this map exceeds certain size bounds, e.g. 1000, 10k, 100k.

done, I added a redis command that can query the DEBUG_STRING

ericl · 2019-02-11T23:48:59Z

Could you also add

import ray

@ray.remote
def f():
    return 0

@ray.remote
def g():
    import time
    start = time.time()
    while time.time() < start + 1:
        ray.get([f.remote() for _ in range(10)])

# 10MB -> hangs after ~5 iterations
# 20MB -> hangs after ~20 iterations
# 50MB -> hangs after ~50 iterations
ray.init(redis_max_memory=1024 * 1024 * 10)

i = 0
for i in range(50):
    i += 1
    a = g.remote()
    [ok], _ = ray.wait([a])
    print("iter", i)

to the jenkins tests? This will hang-fail before this fix.

robertnishihara · 2019-02-12T00:00:57Z

@pcmoritz @ericl how long does that take to run assuming it's successful? The Jenkins tests take quite a long time as it is.

ericl · 2019-02-12T00:01:52Z

It takes about 30 seconds.

robertnishihara · 2019-02-12T00:02:26Z

src/ray/gcs/redis_module/ray_redis_module.cc

  RedisModuleString *channel;
-  RAY_RETURN_NOT_OK(FormatPubsubChannel(&channel, ctx, pubsub_channel_str, keyname));
+  RAY_CHECK_OK(FormatPubsubChannel(&channel, ctx, pubsub_channel_str, keyname));


@guoyuhong removed all RAY_CHECK calls in #3855. So let's keep it that way.

good idea, done

AmplabJenkins · 2019-02-12T00:42: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/11800/
Test FAILed.

ericl

LGTM

AmplabJenkins · 2019-02-12T02:11:36Z

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

guoyuhong · 2019-02-12T03:47:37Z

src/ray/gcs/redis_module/ray_redis_module.cc

+      // therefore we construct a temporary redis string here, which
+      // will be garbage collected by redis.
+      auto channel = RedisModule_CreateString(
+          ctx, client_channel.data(), client_channel.size());


@pcmoritz Can we delete this temporary redis string immediately after PUBLISH command finishes? In our system we do not enable redis LRU by default. That may cause redis memory leak.

Are you talking about LRU eviction or about automatic memory management? We shouldn’t have to free strings or close keys because redis automatic memory management takes care of that. If you do need to free strings then we you would need to do it in many places.

Sorry, I thought it was a key... It is just a Redis String, which is fine.

guoyuhong · 2019-02-12T03:54:33Z

One more question, what is the typical request key number in the key value of the map std::unordered_map<std::string, std::vector<std::string>> notification_map;. If the number is not big, it is fine. Otherwise, std::remove may be very slow for a vector, since we often call CancelNotifications in task table and object table.

AmplabJenkins · 2019-02-12T04:01: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/11807/
Test FAILed.

AmplabJenkins · 2019-02-12T04:14: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/11808/
Test FAILed.

AmplabJenkins · 2019-02-12T04:20: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/11809/
Test FAILed.

ericl · 2019-02-12T08:31:24Z

jenkins retest this please

robertnishihara · 2019-02-12T08:38:38Z

src/ray/gcs/redis_module/ray_redis_module.cc

      if (reply == NULL) {
        return RedisModule_ReplyWithError(ctx, "error during PUBLISH");
      }
    }
+    notification_map.erase(it);


Can you explain this line? You said it was a bug, but shouldn’t we only update this when a client calls subscribe or unsubscribe?

This was a fluke, I just had forgotten how to erase from a c++ vector properly

AmplabJenkins · 2019-02-12T11:36:36Z

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

ericl · 2019-02-12T17:51:50Z

jenkins retest this please

Also some lint errors...

AmplabJenkins · 2019-02-12T23:35: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/11836/
Test FAILed.

AmplabJenkins · 2019-02-13T00:53:15Z

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

pcmoritz · 2019-02-13T02:54:07Z

@guoyuhong The number of elements in the vector will be fairly small (= the number of clients subscribed to that key), probably < 100 in most cases. I expect vectors to be faster in this case than other datastructures.

guoyuhong

@pcmoritz Thanks for the confirmation.

AmplabJenkins · 2019-02-13T05:07: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/11851/
Test FAILed.

AmplabJenkins · 2019-02-13T05:48:41Z

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

AmplabJenkins · 2019-02-13T06:24: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/11858/
Test FAILed.

ericl · 2019-02-13T07:11:24Z

jenkins retest this please

ericl · 2019-02-13T07:13:35Z

OOM command not allowed when used memory > 'maxmemory'.

Odd this happens when the test is initializing, perhaps 10MB is too small on the jenkins environment.

AmplabJenkins · 2019-02-13T10:08:11Z

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

AmplabJenkins · 2019-02-13T10:12: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/11862/
Test FAILed.

AmplabJenkins · 2019-02-13T10:19:08Z

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

pcmoritz · 2019-02-13T20:38:33Z

Jenkins retest this please

AmplabJenkins · 2019-02-13T20:59: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/11891/
Test FAILed.

AmplabJenkins · 2019-02-14T01:36: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/11908/
Test FAILed.

pcmoritz · 2019-02-14T05:36:36Z

alright, looks like Jenkins is finally happy (the multinode_test script might not pass through the flag to redis, it's a little strange but also not the intended use case...)

AmplabJenkins · 2019-02-14T06:12:10Z

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

ericl · 2019-02-14T06:20:20Z

Ok, no luck with jenkins. I'm going to merge this and we can figure out the test issue separately.

AmplabJenkins · 2019-02-14T06:39: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/11933/
Test FAILed.

convert notification_key map to C++ datastructure

bde59b4

ericl reviewed Feb 11, 2019

View reviewed changes

robertnishihara reviewed Feb 12, 2019

View reviewed changes

pcmoritz added 4 commits February 11, 2019 16:45

fix crash and add debug string

4ecdc18

clean notification map up (this was a bug before)

4c62c3b

remove checks

7723fca

add jenkins test

c76e55d

ericl approved these changes Feb 12, 2019

View reviewed changes

guoyuhong reviewed Feb 12, 2019

View reviewed changes

robertnishihara reviewed Feb 12, 2019

View reviewed changes

pcmoritz added 2 commits February 12, 2019 12:33

linting

7ae7e13

fixes

7ef5c8e

pcmoritz added 2 commits February 12, 2019 18:41

properly erase

e928b95

clean up

92f001e

guoyuhong approved these changes Feb 13, 2019

View reviewed changes

linting

28b0cf7

Update test_wait_hanging.py

ac5ac49

Update run_multi_node_tests.sh

13555d7

increase redis_max_memory

403bec5

pcmoritz added 2 commits February 13, 2019 19:19

fix dat jenkins

6a87ef7

update

be25d07

Update run_multi_node_tests.sh

48973b7

ericl merged commit 810cc17 into ray-project:master Feb 14, 2019

robertnishihara deleted the fix-wait branch February 15, 2019 20:58

Fix LRU eviction of client notification datastructure #4021

Fix LRU eviction of client notification datastructure #4021

Conversation

pcmoritz commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmoritz Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

ericl commented Feb 11, 2019

robertnishihara commented Feb 12, 2019

ericl commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Feb 12, 2019

ericl left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guoyuhong commented Feb 12, 2019

AmplabJenkins commented Feb 12, 2019

AmplabJenkins commented Feb 12, 2019

AmplabJenkins commented Feb 12, 2019

ericl commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Feb 12, 2019

ericl commented Feb 12, 2019

AmplabJenkins commented Feb 12, 2019

AmplabJenkins commented Feb 13, 2019

pcmoritz commented Feb 13, 2019 • edited Loading

guoyuhong left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

ericl commented Feb 13, 2019

ericl commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

pcmoritz commented Feb 13, 2019

AmplabJenkins commented Feb 13, 2019

AmplabJenkins commented Feb 14, 2019

pcmoritz commented Feb 14, 2019

AmplabJenkins commented Feb 14, 2019

ericl commented Feb 14, 2019

AmplabJenkins commented Feb 14, 2019

pcmoritz Feb 12, 2019 •

edited

Loading

pcmoritz commented Feb 13, 2019 •

edited

Loading