Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options ...................................................................... Patch Set 3: (3 comments) A few more comments. Planning to take another look later today. http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc File be/src/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@395 PS2, Line 395: } : string error_msg = tokens.size() == 3 ? : tokens[2] : > I'm not sure what you mean. Its a property of this particular debug action Yeah, didn't see the check for "iequals(cmd, "FAIL")" above. Although, I think the pattern in Impala is to document the return type for each method, so would be nice to document that for this method as well. http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@92 PS2, Line 92: <global label>:<arg0>:...:<argN>:<command>@<param0>@<param1>@...<paramN>" > Sure, since the debug action in this patch is being passed in as a command Yeah, maybe passing in a path to a file that contains JSON is the right way to do it. Just a thought really. I think another way to make it easier to understand how to use DEBUG_ACTIONS, would be to include some examples. Can you add a few? At least, a few relevant to the changes you are making. http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14641/3/common/thrift/ImpalaService.thrift@101 PS3, Line 101: and error should mention that the DebugAction might change code paths depending on the value of error - e.g. in impala-service-pool.cc QueueInboundCall, it either calls FailAndReleaseRpc or RejectTooBusy depending on the value of error. Right now, it sounds like it just returns a Status(error), and thats it. -- 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: 3 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: Tue, 12 Nov 2019 17:31:00 +0000 Gerrit-HasComments: Yes
