balancer: move Balancer and Picker to V2; delete legacy API #3431
Conversation
| } | ||
| // err is some other error. | ||
| return nil, nil, status.Error(codes.Unknown, err.Error()) | ||
| return nil, nil, status.Error(codes.Unavailable, err.Error()) |
There was a problem hiding this comment.
What if err is a status error?
If we want to always have code unavailable, what if err is a status with unavailable?
There was a problem hiding this comment.
It's been awhile, but this is the behavior we agreed on (and is documented as such in balancer.Picker.Pick). This would be a fail-fast RPC with an error this is not a "drop" error or ErrNoSubConnAvailable.
We could make this detect status errors and only grab the message. I think the idea was that there would be no reason to return a status in this scenario - this is for things like "all subconns in transient failure" or "resolver returned no addresses". We can talk more about this offline.
There was a problem hiding this comment.
As discussed offline, changed so DropRPCError happens automatically for status errors. PTAL. If this looks good to you, I'll file an issue to remove our uses of ErrTransientFailure (they are somewhat pervasive right now).
|
Pending Travis as a sanity check, this should be ready to review again @menghanl. |
The only substantial change made vs. the previous attempt at this PR (#3301) is to change
TestEmptyAddrsto expect both pick first and round robin to continue using previous addresses, in light of a behavior change made about a month ago. Note that this appears likely to change again in the future (so that both balancers honor the "no addresses" update), depending upon the direction of current design discussions -- but I do not want to make any behavior changes as part of this PR.#3180