[GCS]Delete redis accessor module by kisuke95 · Pull Request #9898 · ray-project/ray

kisuke95 · 2020-08-04T06:24:24Z

Why are these changes needed?

Delete the old rediss accessor module, which is not needed. Now, we use GCS table storage instead to handle requests.

In order to remove RedisGcsClient, there still some test files to be modified.

object_manager_stress_test.cc
object_manager_test.cc
object_manager_integration_test.cc

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

AmplabJenkins · 2020-08-04T06:28:50Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-08-04T06:34: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/29357/
Test FAILed.

AmplabJenkins · 2020-08-05T03:50: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/29423/
Test FAILed.

CI_KEEP_ALIVE: Travis

AmplabJenkins · 2020-08-05T09:05: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/29434/
Test FAILed.

src/ray/core_worker/actor_manager.cc

src/ray/gcs/redis_gcs_client.cc

ffbin · 2020-08-06T01:58:28Z

RedisGcsClient does not need to inherit GcsClient, and maybe we can remove it, because we can use RedisClient directly. @rkooo567 What's your suggestion?

rkooo567 · 2020-08-06T08:00:46Z

@kisuke95 Can you update the description?

@ffbin Isn't it going to cause troubles when we support multiple backends if we just directly use RedisClient?

AmplabJenkins · 2020-08-06T13:18: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/29505/
Test FAILed.

ffbin · 2020-08-07T01:52:54Z

@kisuke95 Can you update the description?

@ffbin Isn't it going to cause troubles when we support multiple backends if we just directly use RedisClient?

RedisGcsClient is compatible with redis module. We now use ServiceBasedGcsClient, so RedisGcsClient can be removed.

rkooo567

@ffbin Ah, I see. So you mean we can just use ServiceBasedAccessor for testing and completely remove RedisGcsClient because we won't need it anymore right? It seems like gcs_server -> Redis is done by redis_store_client, so I think it is fine to remove.

Also, it seems like we are writing mock accessor per tests. Is there any way we can just collect all mock accessors in a single file (or dir) and just import them to test files? I assume they are usually resuable.

AmplabJenkins · 2020-08-11T13:30: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/29730/
Test FAILed.

rkooo567 · 2020-08-11T16:34:23Z

Hey @kisuke95 If the PR is ready to be reviewed again, please remove the tag (author action required)!

rkooo567

LGTM in general! Just a few small comments. I will let @ffbin sign off the PR since he should be more familiar with this code path.

src/ray/gcs/test/mock_accessor_test.h

rkooo567 · 2020-08-12T17:09:04Z

src/ray/object_manager/test/object_manager_stress_test.cc

  return ms_since_epoch.count();
 }

+class MockGcsClient : public gcs::RedisGcsClient {


@ffbin Didn't you mention we should use RedisClient directly? Is it still applied?

I think we can remove RedisGcsClient directly.

@kisuke95 Can you address this part?

kisuke95 · 2020-08-13T02:45:34Z

Also, it seems like we are writing mock accessor per tests. Is there any way we can just collect all mock accessors in a single file (or dir) and just import them to test files? I assume they are usually resuable.

@rkooo567 So I put them in a header file, and import it in *.test.cc file

rkooo567 · 2020-08-14T06:57:29Z

I will mark the PR as author action required as @ffbin said we can remove RdisGCSClient. I think if we don't do this in this PR, we will never do that, so I am favorable doing it if it can remove good amount of code.

Please remove the tag if you think it is unnecessary (or if you remove RedisGCSClient).

kisuke95 · 2020-08-14T08:12:20Z

I will mark the PR as author action required as @ffbin said we can remove RdisGCSClient. I think if we don't do this in this PR, we will never do that, so I am favorable doing it if it can remove good amount of code.

Please remove the tag if you think it is unnecessary (or if you remove RedisGCSClient).

@rkooo567 I'm an intern of Alipay, now I'm preparing for my report on work to become a regular staff. I will remove RedisGcsClient after it.

rkooo567 · 2020-08-15T00:40:41Z

No worries @kisuke95 Good luck on your report :)! Please take your time and re-ping me or remove a author-action-required tag whenever you are ready.

CI_KEEY_ALIVE: Travis

kfstorm · 2020-11-03T07:05:44Z

@ffbin Will somebody take on this work?

rkooo567 · 2020-11-03T08:02:59Z

@kfstorm thanks for bringing this up. I’d like to finish this pr asap.

ffbin · 2020-11-03T09:40:19Z

@ffbin Will somebody take on this work?

We will continue to complete this pr. I prefer to split this pr. At present, there are many modified PR files and the merging cycle is relatively long.

clay4megtr · 2020-12-18T05:57:23Z

I will task over this work, cc @ffbin @rkooo567 @kfstorm

kisuke95 added 11 commits July 31, 2020 15:05

error info pubsub

631fa30

fix test

0602f5b

fix doc build error

d34209a

fix test error

d44ae77

fix

5ad2157

fix

7813838

address review

6e4fb1b

fix conflicts

c7165e2

update

22c6d40

Merge branch 'master' into delete_redis_module

9c5760a

delete redis module

4b9f790

rkooo567 self-assigned this Aug 4, 2020

fix CI_KEEP_ALIVE: Travis

2540247

ffbin self-assigned this Aug 5, 2020

ffbin self-requested a review August 5, 2020 07:00

fix object manager test

2a8a585

CI_KEEP_ALIVE: Travis

kisuke95 changed the title ~~[WIP] Delete redis module~~ Delete redis module Aug 5, 2020

ffbin changed the title ~~Delete redis module~~ [GCS]Delete redis module Aug 6, 2020

ffbin reviewed Aug 6, 2020

View reviewed changes

src/ray/core_worker/actor_manager.cc Outdated Show resolved Hide resolved

ffbin reviewed Aug 6, 2020

View reviewed changes

src/ray/gcs/redis_gcs_client.cc Outdated Show resolved Hide resolved

CI_KEEP_ACTIVE: Travis

b211773

rkooo567 reviewed Aug 7, 2020

View reviewed changes

kisuke and others added 4 commits August 12, 2020 10:03

CI_KEEP_ALIVE: Travis

9db4474

Merge branch 'master' into delete_redis_module

62f9f75

.

ca3e769

fix lint

ccde811

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

kisuke95 requested a review from rkooo567 August 12, 2020 09:20

rkooo567 reviewed Aug 12, 2020

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 12, 2020

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

kisuke95 changed the title ~~[GCS]Delete redis module~~ [GCS]Delete redis accessor module Aug 13, 2020

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, 2020

kisuke95 added 6 commits August 20, 2020 10:14

Merge branch 'master' into delete_redis_module

430b2b9

remove RedisGcsClient from some test file

1829618

.

7ea3111

.

39a2465

Merge branch 'master' into delete_redis_module

8691851

CI_KEEY_ALIVE: Travis

fix mistake

ba3866f

clay4megtr self-assigned this Dec 18, 2020

clay4megtr mentioned this pull request Dec 21, 2020

[GCS] Delete redis gcs client and redis_xxx_accessor #12996

Merged

6 tasks

clay4megtr closed this Dec 24, 2020

Conversation

kisuke95 commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

AmplabJenkins commented Aug 4, 2020

Uh oh!

AmplabJenkins commented Aug 4, 2020

Uh oh!

AmplabJenkins commented Aug 5, 2020

Uh oh!

AmplabJenkins commented Aug 5, 2020

Uh oh!

Uh oh!

Uh oh!

ffbin commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rkooo567 commented Aug 6, 2020

Uh oh!

AmplabJenkins commented Aug 6, 2020

Uh oh!

ffbin commented Aug 7, 2020

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Aug 11, 2020

Uh oh!

rkooo567 commented Aug 11, 2020

Uh oh!

rkooo567 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rkooo567 Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

ffbin Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

rkooo567 Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

kisuke95 commented Aug 13, 2020

Uh oh!

rkooo567 commented Aug 14, 2020

Uh oh!

kisuke95 commented Aug 14, 2020

Uh oh!

rkooo567 commented Aug 15, 2020

Uh oh!

kfstorm commented Nov 3, 2020

Uh oh!

rkooo567 commented Nov 3, 2020

Uh oh!

ffbin commented Nov 3, 2020

Uh oh!

clay4megtr commented Dec 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kisuke95 commented Aug 4, 2020 •

edited

Loading

ffbin commented Aug 6, 2020 •

edited

Loading