core: Don't set firstPass=true on address update in pick_first#12679
Open
ejona86 wants to merge 2 commits intogrpc:masterfrom
Open
core: Don't set firstPass=true on address update in pick_first#12679ejona86 wants to merge 2 commits intogrpc:masterfrom
ejona86 wants to merge 2 commits intogrpc:masterfrom
Conversation
Updating the addresses has no bearing if pick_first is doing the first pass, for the sake of when refreshNameResolution() is triggered. In the first pass pick_first is in CONNECTING and after the first pass it is in TRANSIENT_FAILURE. If pick_first already did a first pass and is in TF, then when it gets updated addresses it _stays_ in TF and the regular "We will count the number of connection failures, and when that number reaches the number of subchannels, we will request re-resolution" should apply. Setting firstPass=true on each address update can cause us to request refreshes too frequently. So we only need to adjust firstPass when changing the rawConnectivityState. And if we do that, we don't actually need firstPass; we can use numTf instead.
Member
Author
|
I had some discussion today with @dfawley and @murgatroid99, and they both think this is working as designed. I don't believe that to be the intention, and gRFC A61 is not precise on the matter, but it'd be fair to update the gRFC if we do go this way. I'm going to make an alternative change that only impacts serializeRetries, since that's our Java's own creation. |
Member
Author
|
Reopened because I'm going to send out a tweak to the gRFC. (gRFC update sent out: grpc/proposal#539) |
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
1 participant
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.
grpc/proposal#539 updated gRFC A61 to continue
counting numTf while in first pass and to only conditionally trigger
name resolution refresh if there have been sufficient connection
failures. This prevents triggering a refresh on address updates that
don't actually change the addresses. DNS would be discarding some of
these refreshes, but if we actually implemented refreshing DNS like
other languages then it'd form a loop. (grpc-java discards refresh
requests if they are too soon after the last result; other languages
delay such requests until suffient time has passed.)
With the new gRFC language, we no longer have to have a separate
firstPass variable; the isPassComplete() function is sufficient on its
own.