endpointsharding: cast EndpointMap values to *balancerWrapper instead of Balancer #8069
Conversation
arjan-bal
added
Type: Bug
Area: Resolvers/Balancers
arjan-bal
changed the title
arjan-bal
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8069 +/- ##
==========================================
+ Coverage 82.25% 82.38% +0.12%
==========================================
Files 387 387
Lines 39040 39065 +25
==========================================
+ Hits 32114 32182 +68
+ Misses 5593 5557 -36
+ Partials 1333 1326 -7
|
arjan-bal
changed the title
|
|
||
| // Execute the resolver error path. pickfirst will ignore the resolver error | ||
| // as it has a good resolver state. | ||
| mr.ReportError(errors.New("test error")) |
There was a problem hiding this comment.
This tests that the operation was a nop, which isn't really a good test of the functionality. I.e. the child could not even get the call and the test would still pass. I think it would be better to have a test that calls ResolverError before it's working and ensures that it results in pick errors / TF state.
There was a problem hiding this comment.
The children of enpointsharding wouldn't get created if the resolver doesn't produce a good list of endpoints first.
I can use a stub child balancer which modifies pickfirst's behaviour of ignoring resolver errors when it has a working state.
There was a problem hiding this comment.
Good point. Or you should be able to make it start failing (disconnect subchannel) and then produce a ResolverError. The errors from pick should change IIUC
There was a problem hiding this comment.
I stopped the backends, reported a resolver error and verified the RPC error message.
arjan-bal
force-pushed
the
endpointsharding-fix-resolver-error
branch
from
6234f46 to
28b14ff
Compare
arjan-bal
force-pushed
the
endpointsharding-fix-resolver-error
branch
from
28b14ff to
a96f1db
Compare
| cc, err := grpc.NewClient(mr.Scheme()+":///", grpc.WithResolvers(mr), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| dOpts := []grpc.DialOption{ | ||
| grpc.WithResolvers(mr), grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| // Use a large backoff dealy to avoid the error picker being updated |
| // too quickly. | ||
| grpc.WithConnectParams(grpc.ConnectParams{ | ||
| Backoff: backoff.Config{ | ||
| BaseDelay: 100 * time.Second, |
There was a problem hiding this comment.
Nit/Optional: This could be 2*defaultTestTimeout instead of an arbitrarily large value.
| for ; ctx.Err() == nil; <-time.After(time.Millisecond) { | ||
| _, err := client.EmptyCall(ctx, &testpb.Empty{}) | ||
| if err == nil { | ||
| t.Fatalf("EmptyCall returned unexpected error: <nil>, want %q", "test error") |
There was a problem hiding this comment.
Nit: instead of saying unexpected error nil, it could say succeeded when expected to fail with "test error"
There was a problem hiding this comment.
Changed the error message.
3 participants
In #8031, the
balancer.Balancerembedding inbalancerWrapperwas removed. This causesendpointshardingto panic when casting theEndpointMapvalue to abalancer.Balancer. This PR fixes the bug and adds a test.RELEASE NOTES: None