balancer/endpointsharding: Ignore empty endpoints #7674
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7674 +/- ##
==========================================
- Coverage 82.01% 81.78% -0.24%
==========================================
Files 361 361
Lines 27819 27830 +11
==========================================
- Hits 22815 22760 -55
- Misses 3823 3868 +45
- Partials 1181 1202 +21
|
| // Check/return early if any endpoints have no addresses. | ||
|
|
||
| addrs := resolver.NewAddressMap() | ||
| // Check/return early if any endpoints have no addresses, or there is a duplicate address map. |
|
Added a unit test, thanks. |
| es := NewBalancer(nil, balancer.BuildOptions{}) | ||
| addr := resolver.Address{Addr: "addr1"} | ||
| err := es.UpdateClientConnState(balancer.ClientConnState{ | ||
| ResolverState: resolver.State{ |
There was a problem hiding this comment.
This is pretty limited in what it covers. E.g. it doesn't cover addresses across endpoints, or endpoints with multiple addresses. Please add some more test cases to cover more possibilities -- probably make it table driven where []Endpoint is the input and the result is probably just a bool to indicate whether there should be an error or not.
There was a problem hiding this comment.
Added more coverage for passing and failing tests.
| {Addresses: []resolver.Address{addr1, addr2}}, | ||
| {Addresses: []resolver.Address{}}, | ||
| }, | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
Please add a couple more:
- no error with addr1+2 and addr3+4 in separate endpoints -- you should generally always have a "pass" test.
- error where endpoint1 has the same addr multiple times
There was a problem hiding this comment.
Discussed offline; had the first case. Added the second case.
|
Updated endpointsharding as per offline discussion:
|
| if len(endpoints) == 0 { | ||
| return errors.New("endpoints list is empty") | ||
| } |
There was a problem hiding this comment.
I think you can just delete this? It's a subset of the other validation?
There was a problem hiding this comment.
I kept this here soley for the sake of the more specific error string. I think the two are distinct, which is why I left this clause for a different error message.
| // ValidateEndpoints returns an error if the endpoints list is empty, or no | ||
| // addresses are present in endpoint list. |
There was a problem hiding this comment.
This comment should indicate how/why/when to use this, not what it does.
// The EndpointSharding balancer assumes ValidateEndpoints has been used to validate all Endpoints
// passed to UpdateCLientConnState.?
Or is this scenario actually not a problem for the endpointsharding balancer itself? In which case maybe this function belongs in package resolver or balancer?
There was a problem hiding this comment.
Yeah I can move it to resolver or balancer, as this should trigger failure case in all petiole policies. Mark - "All petiole LB policies reject updates with no addresses."
There was a problem hiding this comment.
But also since it's gatekeeped in all petiole policies maybe it does make sense to keep it here? Although looks like we won't use this in petiole like ring hash so I'll move it to resolver.
There was a problem hiding this comment.
Updated comment as well, let me know what you think.
| // UpdateClientConnState creates a child for new endpoints and deletes children | ||
| // for endpoints that are no longer present. It also updates all the children, | ||
| // and sends a single synchronous update of the childrens' aggregated state at | ||
| // the end of the UpdateClientConnState operation. If any endpoint has no | ||
| // addresses it will ignore that endpoint. Otherwise, returns first error found | ||
| // from a child, but fully processes the new update. |
There was a problem hiding this comment.
Nobody will see this godoc comment here since it's on an unexported type.
There was a problem hiding this comment.
Are you ok with that?
zasweq
changed the title
This PR ignores empty endpoints in endpoint sharding, and adds a validation helper in the resolver package that all petiole policies should call.
RELEASE NOTES: N/A