-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
interop: update --fail_on_failed_rpc to wait for initial success #3763
interop: update --fail_on_failed_rpc to wait for initial success #3763
Conversation
interop/xds/client/client.go
Outdated
@@ -58,6 +58,7 @@ var ( | |||
mu sync.Mutex | |||
currentRequestID int32 | |||
watchers = make(map[statsWatcherKey]*statsWatcher) | |||
rpcSucceeded bool |
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.
There's no atomic boolean, but you can use int + atomic, like this: https://github.com/grpc/grpc-go/blob/master/internal/profiling/profiling.go#L49
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.
done
interop/xds/client/client.go
Outdated
) | ||
|
||
type statsService struct { | ||
testpb.UnimplementedLoadBalancerStatsServiceServer | ||
} | ||
|
||
func HasRpcSucceeded() bool { |
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.
No need to export this and the other function.
RPC
, not Rpc
, otherwise go lint will complain.
And call this setRPCSucceeded()
?
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.
Awesome, I am up to page 3 of a tour of Go now: https://tour.golang.org/basics/3 :-)
Done (I think!)
10bf0ee
to
8e08486
Compare
merged this with master to incorporate the new changes from #3737 |
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
Analogous to grpc/grpc#23629
cc @menghanl