Skip to content
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.

Sign up for GitHub

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

Jump to bottom

xds interop: update interop client to support new tests #3737

Merged
merged 4 commits into from
Jul 28, 2020
Merged

xds interop: update interop client to support new tests #3737

merged 4 commits into from
Jul 28, 2020

Conversation

menghanl
Copy link
Contributor

  • Add flags --rpc and --metadata
  • Update stats service to report rpc_by_type
  • Change client and server to read/send hostname in metadata, instead of RPC response

@menghanl menghanl requested a review from ericgribkoff July 10, 2020 19:19
@menghanl menghanl force-pushed the xds_interop_client_rpc_metadata branch from 2c37ee6 to 86121dc Compare July 13, 2020 22:23
@ericgribkoff ericgribkoff mentioned this pull request Jul 14, 2020
Merged
@menghanl menghanl force-pushed the xds_interop_client_rpc_metadata branch from dbac4a0 to 78a567e Compare July 15, 2020 01:23
@menghanl menghanl assigned menghanl and unassigned ericgribkoff Jul 21, 2020
@menghanl menghanl force-pushed the xds_interop_client_rpc_metadata branch 4 times, most recently from 5b7cbf7 to 7bb7e8a Compare July 22, 2020 23:17
@menghanl menghanl added this to the 1.31 Release milestone Jul 23, 2020
@menghanl menghanl force-pushed the xds_interop_client_rpc_metadata branch from 5eff640 to 094d304 Compare July 24, 2020 17:54
dfawley
dfawley reviewed Jul 27, 2020
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few nits.

interop/xds/client/client.go Outdated
}

func (watcher *statsWatcher) buildResp() *testpb.LoadBalancerStatsResponse {
rpcsByType := make(map[string]*testpb.LoadBalancerStatsResponse_RpcsByPeer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, but usually a good idea: make(map[]..., len(watcher.rpcsByType))

...or, would it be better to store rpcsByType as map[type-string]RpcsByPeer so it doesn't need to be re-formed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added length.
And removed rpcType all together.

interop/xds/client/client.go Outdated
Comment on lines 166 to 169
case "UnaryCall":
ret = append(ret, unaryCall)
case "EmptyCall":
ret = append(ret, emptyCall)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case "UnaryCall", "EmptyCall": ret = append(ret, rpcType(r))?

Copy link
Contributor Author

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
func parseRPCTypes(rpcStr string) (ret []rpcType) {
rpcs := strings.Split(rpcStr, ",")
if len(rpcs) == 0 {
return []rpcType{"UnaryCall"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return []rpcType{unaryCall}

Copy link
Contributor Author

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

func parseRPCTypes(rpcStr string) (ret []rpcType) {
rpcs := strings.Split(rpcStr, ",")
if len(rpcs) == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: if len(rpcStr) == 0 and move above the Split.

Copy link
Contributor Author

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
}
rpcsToMD[rmSplit[0]] = append(rpcsToMD[rmSplit[0]], rmSplit[1:]...)
}
var ret []*rpcConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ret := make([]*rpcConfig, len(rpcs))

Copy link
Contributor Author

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
Comment on lines 238 to 241
var (
p peer.Peer
header metadata.MD
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are going to use a var block, maybe move it down to where err is declared and include that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

menghanl added 3 commits July 27, 2020 15:26
@menghanl
[xds_interop_client_rpc_metadata] rpc and stats
dd2de94 
add empty call to xds interop client/server
add rpcs by type to stats rpc

[xds_interop_client_rpc_metadata] xds interop client --metadata

[xds_interop_client_rpc_metadata] proto doc

[xds_interop_client_rpc_metadata] lint

[xds_interop_client_rpc_metadata] proto field order

[xds_interop_client_rpc_metadata] proto message name

[xds_interop_client_rpc_metadata] by_type -> by_method
@menghanl
[xds_interop_client_rpc_metadata] unarycall also checks hostname from…
f7de75a 
… response
@menghanl
[xds_interop_client_rpc_metadata] c1
b8f0a0b
@menghanl menghanl force-pushed the xds_interop_client_rpc_metadata branch from 094d304 to b8f0a0b Compare July 27, 2020 22:43
dfawley
dfawley approved these changes Jul 27, 2020
View reviewed changes
interop/xds/client/client.go Outdated

func parseRPCTypes(rpcStr string) (ret []string) {
if len(rpcStr) == 0 {
return []string{"UnaryCall"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unaryCall

Copy link
Contributor Author

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
rpcs := strings.Split(rpcStr, ",")
for _, r := range rpcs {
switch r {
case "UnaryCall", "EmptyCall":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case unaryCall, emptyCall

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@menghanl menghanl merged commit 8b7764b into grpc:master Jul 28, 2020
@menghanl menghanl deleted the xds_interop_client_rpc_metadata branch July 28, 2020 01:27
@ericgribkoff ericgribkoff mentioned this pull request Jul 28, 2020
Merged
menghanl added a commit that referenced this pull request Jul 28, 2020
@menghanl
xds interop: update interop client to support new tests (#3737)
4928046
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

@ericgribkoff ericgribkoff ericgribkoff approved these changes

Assignees

@menghanl menghanl

Labels
Type: Testing
Projects
None yet
Milestone
1.31 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@menghanl @ericgribkoff @dfawley