allow disable core_list setting and override qps server in benchmarks#9168
Conversation
vjpai
left a comment
vjpai
left a comment
There was a problem hiding this comment.
Looks pretty good but raising some small issues.
test/cpp/qps/driver.cc
Outdated
| gpr_free(host); | ||
| gpr_free(driver_port); | ||
| gpr_free(cli_target); | ||
| if (qps_server_target_override != NULL && strlen(qps_server_target_override) > 0) { |
There was a problem hiding this comment.
Since the purpose of this seems to be to work around the gpr_join_host_port business, why not just have an option to disable that? That way you can still have multiple server targets.
There was a problem hiding this comment.
Partly I was trying to avoid host-port parsing, but mostly the benefit was to allow configuring of the qps server target when invoking the driver, rather than from info provided by the worker. I could change the parameter to a list?
test/cpp/qps/driver.cc
Outdated
| // Setup the hosts and core counts | ||
| auto hosts_cores = get_hosts_and_cores(workers); | ||
| std::unordered_map<string, std::deque<int>> hosts_cores; | ||
| if(configure_core_lists) { |
test/cpp/qps/driver.cc
Outdated
|
|
||
| // Setup the hosts and core counts | ||
| auto hosts_cores = get_hosts_and_cores(workers); | ||
| std::unordered_map<string, std::deque<int>> hosts_cores; |
There was a problem hiding this comment.
Maybe this whole variable and its initialization need to be moved down below where it is actually used? That way you are just checking configure_core_lists once down below and then only declaring and setting up this variable if it's needed.
apolcyn
force-pushed
the
allow_disable_qps_json_driver_core_counts
branch
from
5c54b3f to
87636cd
Compare
vjpai
left a comment
vjpai
left a comment
There was a problem hiding this comment.
Sorry, asking for a change back.
test/cpp/qps/driver.cc
Outdated
| } | ||
| if (configure_core_lists) { | ||
| if (i == 0) { | ||
| hosts_cores = get_hosts_and_cores(workers); |
There was a problem hiding this comment.
Oops, I made a bad suggestion. I guess you should just pull this back out of the loop after all; it's ugly under a nested conditional like this.
apolcyn
force-pushed
the
allow_disable_qps_json_driver_core_counts
branch
from
4aacac9 to
4873d30
Compare
|
test failure on known issue: interop PR - #8989 |
|
latest updates rebase and squash |
3 participants
Replaces https://github.com/grpc/grpc/pull/8936/files