clientv3/integration: add TestBalancerUnderServerShutdownWatch #8758
Conversation
|
/cc @jpbetz can you give this a review? |
|
|
||
| // TestBalancerUnderServerFailureWatch expects that watch client | ||
| // switch its endpoints when the member of the pinned endpoint fails. | ||
| func TestBalancerUnderServerFailureWatch(t *testing.T) { |
There was a problem hiding this comment.
failure is too generic: network partition, server shutdown, etc.
| lead := clus.WaitLeader(t) | ||
|
|
||
| // close unnecessary clients | ||
| clus.Client(lead).Close() |
There was a problem hiding this comment.
just start with no client. create one client to do put.
gyuho
changed the title
| cli.SetEndpoints(eps...) | ||
|
|
||
| wch := cli.Watch(context.Background(), "foo", clientv3.WithCreatedNotify()) | ||
| <-wch |
There was a problem hiding this comment.
select on this. we need to have a timeout.
| defer cli.Close() | ||
| cli.SetEndpoints(eps...) | ||
|
|
||
| wch := cli.Watch(context.Background(), "foo", clientv3.WithCreatedNotify()) |
There was a problem hiding this comment.
make both watch key and value consts.
| // switch to others when eps[lead] fails | ||
| for ev := range wch { | ||
| received := false | ||
| for _, e := range ev.Events { |
There was a problem hiding this comment.
why do we need this loop when we only put one key?
|
|
||
| // writes to eps[lead+1] | ||
| for { | ||
| _, err = clus.Client((lead+1)%3).Put(context.Background(), "foo", "bar") |
There was a problem hiding this comment.
give a timeout here instead of using context.Background.
|
|
||
| select { | ||
| case <-donec: | ||
| case <-time.After(5 * time.Second): // enough time for leader election, balancer switch |
There was a problem hiding this comment.
how long is the election timeout?
There was a problem hiding this comment.
10ms for integration package.
There was a problem hiding this comment.
i mean election timeout for the etcdserver. i want to understand why we wait for 5 seconds here, not 10s 20s.
| t.Fatal(err) | ||
| } | ||
| defer cli1.Close() | ||
| cli1.SetEndpoints(eps...) // add all eps |
There was a problem hiding this comment.
// add all eps to list, so that when the original pined one fails the client can switch to other available eps.
| lead := clus.WaitLeader(t) | ||
|
|
||
| // pin eps[lead] | ||
| cli1, err := clientv3.New(clientv3.Config{Endpoints: []string{eps[lead]}}) |
| clus.Members[lead].Terminate(t) | ||
|
|
||
| // writes to eps[lead+1] | ||
| cli2, err := clientv3.New(clientv3.Config{Endpoints: []string{eps[(lead+1)%3]}}) |
|
lgtm after resolving all the comments. leave to @jpbetz for a final approve. |
| // writes to eps[lead+1] | ||
| cli2, err := clientv3.New(clientv3.Config{Endpoints: []string{eps[(lead+1)%3]}}) | ||
| if err != nil { | ||
| t.Fatal(err) |
| if err == rpctypes.ErrTimeout || err == rpctypes.ErrTimeoutDueToLeaderFail { | ||
| continue | ||
| } | ||
| t.Fatal(err) |
There was a problem hiding this comment.
add context to the error. only logging the error msg itself is not super helpful
gyuho
force-pushed
the
failure-test
branch
4 times, most recently
from
c3e9d2e
to
41042ea
Compare
Current Watch integration tests haven't covered the balancer switch behavior under server failures. Signed-off-by: Gyu-Ho Lee <[email protected]>
|
/lgtm Test logic looks correct. I don't have any concerns. Thanks! |
|
SemaphoreCI is unrelated (gRPC issue again). So merging. |
Current Watch integration tests haven't covered the balancer
switch behavior under server failures.
#8711