[core] Refactor how we store data in redis. #46861

rynewang · 2024-07-30T07:49:22Z

Ray GCS Fault Tolerance replies on an external Redis. We write all data for a Ray cluster into a single Redis HASH. Within the HASH, we use a Redis field prefix to represent a Ray table. This is OK for single field CRUD, but for "get all entries for a Ray table" we would have to do a full table HSCAN and filter out other Ray tables by key prefix. This slows things down and the call to get_all_node_info and get_all_actor_info are in fact fairly common.

Updates the Redis usage by creating multiple Redis HASHes, 1 for each Ray table. The single field CRUD are largely the same; but "get all" operations now become a simple HSCAN on the target HASH only. This saves lots of compute.

Closes #46706.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-08-12T21:34:25Z

ping @rkooo567

rkooo567 · 2024-08-13T20:15:41Z

can you link the original issue?

rynewang · 2024-08-13T21:36:06Z

There's no issue; it's from a community PR #46706

rkooo567

Looks good in general. many minor comments.

Besides, how do we test them now?

rkooo567 · 2024-08-13T20:16:20Z

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

@@ -141,7 +141,7 @@ KEYS *
 # 1) "864b004c-6305-42e3-ac46-adfa8eb6f752"

 # Step 6.5: Check the value of the key.
-HGETALL 864b004c-6305-42e3-ac46-adfa8eb6f752
+SCAN 0 MATCH 864b004c-6305-42e3-ac46-adfa8eb6f752*


cc @kevin85421 this is breaking change for kuberay users I think. Would this be okay?

Do we expect users to directly operate ray redis?

I think this change is fine. My only concern is about cleaning up Redis. @rynewang will provide me with a Docker image, and I will manually test it.

Kuberay cleans redis by calling cleanup_redis_storage which I reimplemented so should be OK. I'm giving @kevin85421 a docker image to test

It's not easy to get a Docker image. Let's merge it and then I will use the nightly image to test.

Also cc @edoakes we are using the function to clean up redis or the raw command?

Yea, I checked with @edoakes before, we also use cleanup_redis_storage

src/ray/gcs/store_client/redis_store_client.cc

rkooo567 · 2024-08-14T16:51:06Z

src/ray/gcs/store_client/redis_store_client.cc

-                                   match_pattern,
-                                   "COUNT",
-                                   std::to_string(batch_count)};
+  std::vector<std::string> args = {"HSCAN", table_name_, std::to_string(cursor_.value())};


why does it not use GenRedisKey?

Ctor of RedisScanner accepts the table name, and all its callers uses GenRedisKey so we are fine.

Do you want a more typed set up, e.g. a struct represending the {external_storage_namespace, table_name} ?

Do you want a more typed set up, e.g. a struct represending the {external_storage_namespace, table_name} ?

Yeah this would be great. I am also confused why this doesn't include external_storage_namespace (that's why I was asking why we don't use GenRedisKey)?

src/ray/gcs/store_client/redis_store_client.cc

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-08-20T22:06:48Z

@rkooo567 PTAL again

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567

Looks good. Last couple comments (please make sure to resolve)!

Regarding Redis, can you sync with @kevin85421 and make sure we recommend to use cleanup_redis_storage , not the raw Redis command? If we have any recommendation using raw Redis command, I think we should have a loud release note.

src/ray/gcs/store_client/redis_store_client.cc

rkooo567 · 2024-08-21T08:01:49Z

src/ray/gcs/store_client/redis_store_client.cc

-                                   match_pattern,
-                                   "COUNT",
-                                   std::to_string(batch_count)};
+  std::vector<std::string> args = {"HSCAN", table_name_, std::to_string(cursor_.value())};


Do you want a more typed set up, e.g. a struct represending the {external_storage_namespace, table_name} ?

Yeah this would be great. I am also confused why this doesn't include external_storage_namespace (that's why I was asking why we don't use GenRedisKey)?

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-08-21T20:26:31Z

Rewrote most of the file to use typed RedisCommand, RedisKey and RedisMatchPattern. PTAL! @rkooo567

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567

looks very good!

python/ray/_private/gcs_utils.py

python/ray/_raylet.pyx

python/ray/includes/global_state_accessor.pxd

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

src/ray/gcs/store_client/redis_store_client.h

src/ray/gcs/store_client/redis_store_client.cc

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

jjyao · 2024-09-12T04:09:50Z

I think the test failure is real (i.e. cannot be fixed by increasing timeout)

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-09-16T20:55:46Z

it IS real. It happens when you have a large BatchDelete, where a bad move on callback function makes the callback never called. fixed and reverted the test relaxings.

jjyao · 2024-09-17T00:38:12Z

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

@@ -136,12 +136,22 @@ export REDIS_POD=$(kubectl get pods --selector=app=redis -o custom-columns=POD:m
 kubectl exec -it $REDIS_POD -- redis-cli -a "5241590000000000"

 # Step 6.4: Check the keys in Redis.
+# Note: the schema changed in Ray 2.36.0. Previously we use a single HASH table,


This needs to be updated to 2.38.0.

angelinalg

stamp

Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

python/ray/_private/gcs_utils.py

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

JoshKarpel · 2024-09-18T23:43:24Z

Hi, just spotted this - wondering if corresponding changes are being made to KubeRay to ensure that it is able to clean up all of these new tables in the GCS cleanup job that runs when a cluster is terminated?

Alternatively, it would be nice if these tables had expirations set (and regularly refreshed) internally so that KubeRay didn't need to clean them up.

See ray-project/kuberay#959

rynewang · 2024-09-19T17:09:19Z

Yes, KubeRay calls a Ray function to clean up tables and that function is updated accordingly. For the table expiration - that’s something separate and yes it’s a good thing to have, though not in this PR.

…

On Wed, Sep 18, 2024 at 16:43 Josh Karpel ***@***.***> wrote: Hi, just spotted this - wondering if corresponding changes are being made to KubeRay to ensure that it is able to clean up all of these new tables in the GCS cleanup job that runs when a cluster is terminated? Alternatively, it would be nice if these tables had expirations set (and regularly refreshed) internally so that KubeRay didn't need to clean them up. See ray-project/kuberay#959 <ray-project/kuberay#959> — Reply to this email directly, view it on GitHub <#46861 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANLX3X4MDUJR2463T43B34DZXIF3JAVCNFSM6AAAAABLVZAYHSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNJZGYZTGNZZHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JoshKarpel · 2024-09-19T22:46:42Z

Awesome, thank you!

initial

090c3e0

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang requested review from architkulkarni, maxpumperla, pcmoritz, kevin85421 and a team as code owners July 30, 2024 07:49

wording

a1fc834

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang mentioned this pull request Jul 30, 2024

[GCS] Optimize Redis query performance with table_name:keys dictionary #46706

Closed

8 tasks

rynewang assigned jjyao and rkooo567 Aug 5, 2024

rkooo567 reviewed Aug 14, 2024

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 14, 2024

fixes

a5dad23

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Aug 20, 2024

lint

35c6776

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang mentioned this pull request Aug 21, 2024

[Request] Test Ray#46861 does not break Kuberay redis cleanup ray-project/kuberay#2319

Open

2 tasks

rynewang changed the title ~~[core] Refactor how we storage things in redis.~~ [core] Refactor how we store things in redis. Aug 21, 2024

rynewang changed the title ~~[core] Refactor how we store things in redis.~~ [core] Refactor how we store data in redis. Aug 21, 2024

rynewang added the go add ONLY when ready to merge, run all tests label Aug 21, 2024

rkooo567 approved these changes Aug 21, 2024

View reviewed changes

typed

dc00698

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

comment example

6272738

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rkooo567 approved these changes Aug 21, 2024

View reviewed changes

jjyao reviewed Aug 21, 2024

View reviewed changes

rynewang enabled auto-merge (squash) August 27, 2024 00:14

lint

bb905a8

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

github-actions bot disabled auto-merge August 27, 2024 00:38

rynewang added 2 commits August 26, 2024 17:39

lint

1ff20d7

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

lint

602f2fa

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao mentioned this pull request Sep 5, 2024

[core][dashboard] push down job_or_submission_id to GCS. #47492

Merged

rynewang added 3 commits September 4, 2024 22:09

Merge remote-tracking branch 'origin/master' into redis-refactor

cf68677

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

fix cpp

4f1b68c

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

remove hand built cleanup

2467782

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang enabled auto-merge (squash) September 5, 2024 05:42

Merge branch 'master' into redis-refactor

a7dc3bd

github-actions bot disabled auto-merge September 5, 2024 22:05

rynewang and others added 2 commits September 6, 2024 11:00

relax the timeout

308cd4a

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

up

c732e11

Signed-off-by: Jiajun Yao <jeromeyjj@gmail.com>

fix bad BatchDelete and revert test relaxing

a7ff71e

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao approved these changes Sep 17, 2024

View reviewed changes

angelinalg approved these changes Sep 17, 2024

View reviewed changes

rynewang added 2 commits September 17, 2024 11:39

Update kuberay-gcs-ft.md 2.38

8b4ed5f

Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Merge branch 'master' into redis-refactor

ca7bcdb

jjyao reviewed Sep 18, 2024

View reviewed changes

rynewang and others added 4 commits September 18, 2024 11:31

Update doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

58cd649

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Update doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

21e4eb5

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

rename func

f263839

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Merge branch 'master' into redis-refactor

3bcc147

jjyao merged commit f298a75 into ray-project:master Sep 18, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Refactor how we store data in redis. #46861

[core] Refactor how we store data in redis. #46861

rynewang commented Jul 30, 2024 •

edited

Loading

rynewang commented Aug 12, 2024

rkooo567 commented Aug 13, 2024

rynewang commented Aug 13, 2024

rkooo567 left a comment

rkooo567 Aug 13, 2024

rynewang Aug 20, 2024

kevin85421 Aug 20, 2024

rynewang Aug 20, 2024 •

edited

Loading

kevin85421 Aug 20, 2024

rkooo567 Aug 21, 2024

jjyao Aug 21, 2024

rkooo567 Aug 14, 2024

rynewang Aug 20, 2024

rynewang Aug 20, 2024

rkooo567 Aug 21, 2024

rynewang commented Aug 20, 2024

rkooo567 left a comment

rkooo567 Aug 21, 2024

rynewang commented Aug 21, 2024

rkooo567 left a comment

jjyao commented Sep 12, 2024

rynewang commented Sep 16, 2024

jjyao Sep 17, 2024

angelinalg left a comment

JoshKarpel commented Sep 18, 2024

rynewang commented Sep 19, 2024 via email

JoshKarpel commented Sep 19, 2024

[core] Refactor how we store data in redis. #46861

[core] Refactor how we store data in redis. #46861

Conversation

rynewang commented Jul 30, 2024 • edited Loading

rynewang commented Aug 12, 2024

rkooo567 commented Aug 13, 2024

rynewang commented Aug 13, 2024

rkooo567 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

rynewang Aug 20, 2024 • 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

Choose a reason for hiding this comment

rynewang commented Aug 20, 2024

rkooo567 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynewang commented Aug 21, 2024

rkooo567 left a comment

Choose a reason for hiding this comment

jjyao commented Sep 12, 2024

rynewang commented Sep 16, 2024

Choose a reason for hiding this comment

angelinalg left a comment

Choose a reason for hiding this comment

JoshKarpel commented Sep 18, 2024

rynewang commented Sep 19, 2024 via email

JoshKarpel commented Sep 19, 2024

rynewang commented Jul 30, 2024 •

edited

Loading

rynewang Aug 20, 2024 •

edited

Loading