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.

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

weightedtarget: return a more meaningful error when no child policy is reporting READY (#5391) #5711

Merged
merged 3 commits into from Oct 17, 2022

Conversation

arvindbr8
Copy link
Member

@arvindbr8 arvindbr8 commented Oct 12, 2022

When all the child policies are in TRANSIENT_FAILURE, the weightedtarget LB policy currently sends a picker which always returns balancer.ErrTransientFailure error message. This error does not provide any info to the user regarding the underlying issue for the failure. With this change we are surfacing the underlying error by sending back the error picker.

Fixes #5391

RELEASE NOTES:

  • xds/weightedtarget: return an error picker to surface a more meaningful error when all children are in transient failure

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 12, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: arvindbr8 / name: Arvind Bright (e363693)

@arvindbr8
Copy link
Member Author

/easycla

@arvindbr8
Copy link
Member Author

/easycla

balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterresolver/priority_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterresolver/priority_test.go Outdated Show resolved Hide resolved
xds/internal/balancer/clusterresolver/priority_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned arvindbr8 and unassigned easwars Oct 13, 2022
@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Oct 13, 2022
balancer/weightedtarget/weightedtarget_test.go Outdated Show resolved Hide resolved
@@ -274,7 +279,8 @@ func (s) TestEDSPriority_SwitchPriority(t *testing.T) {
}

// Should get an update with 1's old picker, to override 2's old picker.
if err := testErrPickerFromCh(cc.NewPickerCh, balancer.ErrTransientFailure); err != nil {
want := errors.New("last connection error: subConn connection error")
if err := testErrPickerFromCh(cc.NewPickerCh, want); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to make the same comment as I did on the other file for here, but realized that testErrPickerFromCh performs a reflect.DeepEqual() on the got and want errors, instead of doing a strings.Contains(). Would you mind fixing the implementation of testErrPickerFromCh to do the latter instead. It doesn't have to be done in this PR. You can send another PR for that after this is merged. Thanks.

@easwars easwars assigned arvindbr8 and unassigned easwars Oct 17, 2022
@arvindbr8 arvindbr8 merged commit eb8aa31 into grpc:master Oct 17, 2022
1 check passed
@arvindbr8 arvindbr8 deleted the 5391 branch October 17, 2022 21:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

weightedtarget: return a more meaningful error when no child policy is reporting READY
2 participants