[ring_hash] update proactive connection attempt logic#38741
Closed
markdroth wants to merge 1 commit into
Closed
Conversation
paulosjca
pushed a commit
to paulosjca/grpc
that referenced
this pull request
Feb 20, 2025
See grpc/proposal#475 for details. While I was at it, I greatly simplified the logic, avoiding an unnecessary duplicate loop over all endpoints that we didn't need to be doing. I removed an unnecessary end2end test that was broken by this change. That test actually should have broken when I first changed ring_hash to delegate to pick_first back in grpc#34244, and I'm not sure why it didn't. But it definitely broke here when I changed the logic to not choose endpoints in any specific order when triggering connection attempts, and since the test was really intended to test a behavior that we no longer guarantee, I removed it. In its place, I added a unit test that shows the new behavior in a much clearer way. (Originally, we didn't have a good way to write unit tests for LB policies, so all of our tests were written as e2e tests. Now that we do have a good framework for unit tests, we've been slowly finding e2e tests that really should be unit tests instead, and moving them over when we find them.) Closes grpc#38741 COPYBARA_INTEGRATE_REVIEW=grpc#38741 from markdroth:ring_hash_tf_and_connecting_fix 6c4c773 PiperOrigin-RevId: 728255272
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See grpc/proposal#475 for details.
While I was at it, I greatly simplified the logic, avoiding an unnecessary duplicate loop over all endpoints that we didn't need to be doing.
I removed an unnecessary end2end test that was broken by this change. That test actually should have broken when I first changed ring_hash to delegate to pick_first back in #34244, and I'm not sure why it didn't. But it definitely broke here when I changed the logic to not choose endpoints in any specific order when triggering connection attempts, and since the test was really intended to test a behavior that we no longer guarantee, I removed it.
In its place, I added a unit test that shows the new behavior in a much clearer way. (Originally, we didn't have a good way to write unit tests for LB policies, so all of our tests were written as e2e tests. Now that we do have a good framework for unit tests, we've been slowly finding e2e tests that really should be unit tests instead, and moving them over when we find them.)