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.

Sign up for GitHub

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

Jump to bottom

[lb pick_first] Enable random shuffling of address list #33254

Merged
merged 20 commits into from Jun 20, 2023
Merged

[lb pick_first] Enable random shuffling of address list #33254

merged 20 commits into from Jun 20, 2023

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented May 25, 2023 •
edited by markdroth

Part of gRFC A62.

@eugeneo eugeneo added the release notes: yes Indicates if PR needs to be in release notes label May 25, 2023
@eugeneo eugeneo requested a review from markdroth as a code owner May 25, 2023 22:58
@markdroth markdroth mentioned this pull request May 26, 2023
Merged
markdroth
markdroth reviewed May 26, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start!

Please let me know if you have any questions. Thanks!

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
markdroth added a commit that referenced this pull request May 26, 2023
@markdroth
[pick_first] add tests that show handling of multiple addresses (#33255)
7741849 
This should help you get an idea of how to write the tests for #33254.
eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Jun 1, 2023
@markdroth @eugeneo
[pick_first] add tests that show handling of multiple addresses (grpc…
2f1439e 
…#33255)

This should help you get an idea of how to write the tests for grpc#33254.
eugeneo
eugeneo commented Jun 5, 2023
View reviewed changes
Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reviews!

test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Show resolved Hide resolved
@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 6, 2023

Unit test should be fixed now. This PR is ready for review.

markdroth
markdroth reviewed Jun 6, 2023
View reviewed changes
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/util/test_lb_policies.cc Outdated Show resolved Hide resolved
eugeneo
eugeneo commented Jun 6, 2023
View reviewed changes
Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review! Updated.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
@eugeneo eugeneo force-pushed the tasks/pick-first/shuffle-config branch from d813ab1 to c74aea1 Compare June 7, 2023 19:41
eugeneo
eugeneo commented Jun 12, 2023
View reviewed changes
Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reviews!

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Show resolved Hide resolved
eugeneo and others added 2 commits June 12, 2023 20:43
markdroth
markdroth reviewed Jun 14, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of remaining issues here.

Please let me know if you have any questions. Thanks!

test/core/client_channel/lb_policy/pick_first_test.cc Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jun 15, 2023
@markdroth @mario-vimal
[pick_first] add tests that show handling of multiple addresses (grpc…
90d35a2 
…#33255)

This should help you get an idea of how to write the tests for grpc#33254.
eugeneo
eugeneo commented Jun 15, 2023
View reviewed changes
Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the reviews. Please take another look.

test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
eugeneo
eugeneo commented Jun 16, 2023
View reviewed changes
Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Updated

test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
test/core/client_channel/lb_policy/pick_first_test.cc Outdated Show resolved Hide resolved
eugeneo and others added 2 commits June 16, 2023 18:03
markdroth
markdroth approved these changes Jun 20, 2023
View reviewed changes
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@eugeneo eugeneo merged commit 7e14a32 into grpc:master Jun 20, 2023
64 of 65 checks passed
@eugeneo eugeneo deleted the tasks/pick-first/shuffle-config branch June 20, 2023 15:41
eugeneo added a commit to eugeneo/grpc that referenced this pull request Jun 20, 2023
@eugeneo
Revert "[lb pick_first] Enable random shuffling of address list (grpc…
0004198 
…#33254)"

This reverts commit 7e14a32.
ctiller pushed a commit that referenced this pull request Jun 20, 2023
@eugeneo
Revert "[lb pick_first] Enable random shuffling of address list (#33254…
d59c8eb 
…)" (#33496)

Original PR: 33254

This reverts commit 7e14a32.
eugeneo added a commit to eugeneo/grpc that referenced this pull request Jun 20, 2023
@eugeneo
Revert "Revert "[lb pick_first] Enable random shuffling of address li…
f4a2bbb 
…st (grpc#33254)" (grpc#33496)"

This reverts commit d59c8eb.
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth approved these changes

Assignees
No one assigned
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@eugeneo @markdroth