balancer/rls: Fix RLS failure mode by treating response with no targets as an error #6735
Conversation
zasweq
changed the title
balancer/rls/picker.go
Outdated
| if allTargetsEmpty { | ||
| if err == nil { | ||
| err = fmt.Errorf("RLS response's target list does not contain any non-empty entries for key %+v", cacheKey) | ||
| } |
There was a problem hiding this comment.
Let's move this if err == nil check to surround all of this new logic?
There was a problem hiding this comment.
Done, alongside implementing offline discussion wrt just checking length of targets slice.
There was a problem hiding this comment.
Updated description.
zasweq
changed the title
There was a problem hiding this comment.
Please update the PR description further, specifically:
-
It still says "no non-empty targets" in multiple places
-
I think you can delete this since the input should be valid and not special?: "There's no good way to test the case where you get targets: []string{""} in my opinion. If you feel strongly about this I can try and figure out a test with the right error code/message here."
|
Updated PR description further. |
Fixes #6719. Previously, the RLS Balancer would not treat an RLS response with no targets as an error. This PR makes it treat an RLS response with no targets as an error.
The relevant language from the RLS LB Design document: "An RLS request is considered to have failed if it returns a non-OK status or the RLS response's targets list is non-empty."
RELEASE NOTES: