-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clientv3/integration: add put blackhole test #8769
Conversation
/cc @jpbetz |
// fail first due to blackhole, retry should succeed | ||
// when gRPC supports better retry on non-delivered request, the first put can succeed. | ||
ctx, cancel = context.WithTimeout(context.Background(), time.Second) | ||
defer cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cancel
is overwritten by line 68?
package main
import (
"context"
"fmt"
"time"
)
func main() {
ctx, cancel := context.WithTimeout(context.Background(), time.Second)
_ = ctx
defer func() {
fmt.Println("cancel 1:", cancel)
cancel()
}()
ctx, cancel = context.WithTimeout(context.Background(), time.Second)
_ = ctx
defer func() {
fmt.Println("cancel 2:", cancel)
cancel()
}()
}
/*
cancel 2: 0x109a950
cancel 1: 0x109a950
*/
Should we move cancel
to line 65?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Confirmed locally that test passes. Semaphore might take awhile due to backlog.
clus := integration.NewClusterV3(t, &integration.ClusterConfig{ | ||
Size: 2, | ||
GRPCKeepAliveMinTime: 1 * time.Millisecond}, | ||
) // avoid too_many_pings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SkipCreatingClient: true,
?
Code looks good. Are there any references to issues or ongoing gPRC work that we can reference from this PR? It might be helpful in the future to have some breadcrumbs tying the gPRC LB work/issues together. |
failfast issue: grpc/grpc-go#1463 testing plan for current balancer: #8711 /cc @jpbetz let me know if you want more reference. |
@xiang90 That's plenty. Thanks! |
/cc @gyuho