Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gcs] Enable grpc based broadcasting by default #19716

Merged
merged 20 commits into from
Nov 3, 2021

Conversation

fishbone
Copy link
Contributor

@fishbone fishbone commented Oct 25, 2021

Why are these changes needed?

This is part of redis removal project. This PR is going to enable grpc based broadcasting by default.

Related issue number

#19438

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/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@fishbone fishbone added do-not-merge Do not merge this PR! @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Oct 25, 2021
@fishbone
Copy link
Contributor Author

This needs to be done before merging this PR:

@fishbone
Copy link
Contributor Author

@fishbone
Copy link
Contributor Author

previously I thought test failure it's related to some unit test, but it looks like system error. merge to master and re-test it.

@fishbone fishbone removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! labels Oct 26, 2021
@fishbone
Copy link
Contributor Author

@rkooo567 we should keep an eye on the nightly tests after this one merged.

@rkooo567
Copy link
Contributor

I think for this particular feature, the "daily" test might give more signals than nightly. (since daily tests have higher stress tests).

Can you also try running them?

@fishbone fishbone added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 26, 2021
@fishbone
Copy link
Contributor Author

Sure, I'll run daily test before merging.

@fishbone
Copy link
Contributor Author

Sadly some daily pg tests failed. Thanks, @rkooo567 for the catching.

@fishbone fishbone added the do-not-merge Do not merge this PR! label Oct 27, 2021
@fishbone
Copy link
Contributor Author

fishbone commented Oct 27, 2021

pg_autoscaling_regression_test
train_small: perf regression
multi_deployment_1k_noop_replica: timeout

RuntimeError: Process timed out while running a command.; Command logs:(ServeController pid=522) 2021-10-26 17:20:17,253	WARNING backend_state.py:1090 -- Deployment 'Echo_98' has 4 replicas that have taken more than 30s to start up. This may be caused by waiting for the cluster to auto-scale, waiting for a runtime environment to install, or a slow constructor. Resources required for each replica: {'CPU': 1}, resources available: {}. component=serve deployment=Echo_98

Wired thing here is that once retry they all passed

@fishbone
Copy link
Contributor Author

@rkooo567 verified
train_small: perf regression
multi_deployment_1k_noop_replica: timeout

are flaky even without this.

I'll add pg-related tests to staging mode and @rkooo567 will get some complicated pg tests (Thanks!!!)

I'll come back to this one later once things are stable.

@fishbone
Copy link
Contributor Author

#19779

@fishbone fishbone removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. do-not-merge Do not merge this PR! labels Nov 2, 2021
@fishbone
Copy link
Contributor Author

fishbone commented Nov 2, 2021

nightly tests look good. merging to trunk and merge after ci

@fishbone
Copy link
Contributor Author

fishbone commented Nov 3, 2021

blocking by this pr #19996

@ericl ericl added the do-not-merge Do not merge this PR! label Nov 3, 2021
@fishbone fishbone removed the do-not-merge Do not merge this PR! label Nov 3, 2021
@fishbone
Copy link
Contributor Author

fishbone commented Nov 3, 2021

merge it since nightly looks good.

@fishbone fishbone merged commit 555b87d into ray-project:master Nov 3, 2021
fishbone added a commit to fishbone/ray that referenced this pull request Nov 4, 2021
@fishbone fishbone deleted the enable-grpc-broadcasting branch November 23, 2021 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants