A61 update: fix ring_hash proactive connection attempt logic #475
Conversation
This file contains 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
dfawley
approved these changes
Feb 14, 2025
ejona86
approved these changes
Feb 14, 2025
There was a problem hiding this comment.
I do think talking so much about priority makes things overly complicated. CONNECTING has always implied "the LB policy is connecting." And since we adopted sticky-TF everywhere, TF also implies "the LB policy is connecting (with backoff)". But I understand why priority is being discussed; it just is unfortunate it is being discussed so much.
This was referenced Feb 17, 2025
copybara-service bot
pushed a commit
to grpc/grpc
that referenced
this pull request
Feb 18, 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 #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 #38741 COPYBARA_INTEGRATE_REVIEW=#38741 from markdroth:ring_hash_tf_and_connecting_fix 6c4c773 PiperOrigin-RevId: 728255272
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
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
Fix logic for ring_hash proactive connection attempts to trigger even if the state update was not triggered by an endpoint entering TRANSIENT_FAILURE state. This addresses two edge cases spotted by @arjan-bal: