Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options ...................................................................... Patch Set 6: (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: ntity, > Please document this arg too. Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/rpc/impala-service-pool.h@115 PS5, Line 115: : /// The address th > These can be const, right ? Done 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 ? As far as I can tell, gcc considers that to be an error - it says "attributes are not allowed on a function-definition" http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@154 PS5, Line 154: const std::vector<s > const std::vector<string>& ? Done 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 ? Done http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@161 PS5, Line 161: > Given the default arg above, is this necessary or is it just for clarity ? Good point, removed. http://gerrit.cloudera.org:8080/#/c/14641/5/be/src/util/debug-util.h@165 PS5, Line 165: ; > Given the default arg above, is this necessary or is it just for clarity ? Done 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 de I wouldn't say that ':' is being re-used for a different purpose here, I would say this patch just extends the existing purpose of ':' to new situations. Or in other words, for the ExecNode actions ':' is used to delimit a list of arguments that must match for the debug action to be executed, just like in this patch. Not a big deal, I can change it if you really find it confusing, but I think it makes more sense this way, and it keeps the parsing code simpler. http://gerrit.cloudera.org:8080/#/c/14641/5/common/thrift/ImpalaService.thrift@105 PS5, Line 105: // omitted, a generic error of the form: 'Debug Action: <label>:<action>' is used. > May help to briefly document the purpose of arg list. Done -- 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: 6 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 23:23:35 +0000 Gerrit-HasComments: Yes
