Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@50 PS5, Line 50: address Please document this arg too. http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@115 PS5, Line 115: std::string hostname_; : std::string port_; These can be const, right ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h File be/src/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@153 PS5, Line 153: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@154 PS5, Line 154: std::vector<string> const std::vector<string>& ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@159 PS5, Line 159: WARN_UNUSED_RESULT nit: Not your change, but can this be moved to the end of the declaration ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@161 PS5, Line 161: std::vector<string>() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@165 PS5, Line 165: std::vector<string>() Given the default arg above, is this necessary or is it just for clarity ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@92 PS5, Line 92: <arg0>:...:<argN> Given that ":" is already used for delimiting different component of the debug action, do you think it may be slightly clearer to not re-use ":" for separating the args ? Instead, can we use "," or other character ? http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@105 PS5, Line 105: // exercising different error paths. May help to briefly document the purpose of arg list. -- To view, visit http://gerrit.cloudera.org:8080/14641 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c047ebce6d32c5ae461f70279391fa2df4c2029 Gerrit-Change-Number: 14641 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Nov 2019 21:31:32 +0000 Gerrit-HasComments: Yes
