xds/clusterimpl: update TestChildPolicyChangeOnConfigUpdate to use custom lb policy. #8730
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8730 +/- ##
==========================================
+ Coverage 83.16% 83.34% +0.17%
==========================================
Files 419 418 -1
Lines 32450 32425 -25
==========================================
+ Hits 26988 27024 +36
+ Misses 4068 4016 -52
+ Partials 1394 1385 -9 🚀 New features to boost your workflow:
|
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
Please ensure that you can run this test 10K or 100K times on Forge before merging this. You could use the instructions here to pull in this change into a piper client and run the test with the race detector turned on (10K/100K times). Thanks.
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
Looks mostly good. Just some minor nits.
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
|
|
There was a problem hiding this comment.
Nit: Please avoid making unnecessary whitespace changes. Thanks.
| }, | ||
| UpdateClientConnState: func(bd *stub.BalancerData, ccs balancer.ClientConnState) error { | ||
| name := pfBuilder.Name() | ||
| name := customLBPolicy |
There was a problem hiding this comment.
Do we need this local variable? Can't this be inlined?
There was a problem hiding this comment.
I think I see why this might be required. This could be because of the use of atomic.Pointer[string]. Could you instead change this to use atomic.Value?
| select { | ||
| case <-ctx.Done(): | ||
| t.Fatalf("Timeout waiting for successful RPC after policy update.") | ||
| return |
There was a problem hiding this comment.
This return statement will never be hit. So, it can be removed.
| _, err := client.EmptyCall(ctx, &testpb.Empty{}) | ||
| if err == nil { |
2 participants
Fixes #8703
Description
This PR fixes the data race in
TestChildPolicyChangeOnConfigUpdate.Changes
pick_firstpolicy (and triggering a race condition viaUnregister), the test now registers a unique custom stub policy namedtest_pick_first.v3xdsxdstypepb.TypedStructand specify the customtype.googleapis.com/test_pick_firstpolicy via TypeURL.RELEASE NOTES: N/A