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 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14641/2//COMMIT_MSG@25 PS2, Line 25: optional 'error > do you mean IMPALA_SERVICE_POOL? Done http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.h@115 PS2, Line 115: std::string hostname_; : std::string p > nit: use std::string Done http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/impala-service-pool.cc@194 PS2, Line 194: mulating rpc errors. To use, specify: : // --debug_actions=IMPALA_SERVICE_POOL:<hostname>:<port>: > would be good to document this some more, it is basically a DebugAction tha Done http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/rpc/rpc-mgr-test.cc@322 PS2, Line 322: Ping > what is Ping suppose to mean? Its the name of a particular rpc that is part of the Ping service, which is a toy service we use for testing, see above in this test. 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] : > should returning tokens[2] only be done if the action is FAIL? might be goo I'm not sure what you mean. Its a property of this particular debug action that it takes the parameter 'error message' and returns it in the error, which is documented here in the comment above and in ImpalaService.thrift. Other debug actions return on OK status. http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@399 PS2, Line 399: : if (ImpaladMetrics::DEBUG_ACTION_NUM_FAIL != nullptr) { : ImpaladMetrics::DEBUG_ACTION_NUM_FAIL->Increment(1l); : } : return Status(TErrorCode::INTERNAL_ERROR, error_msg); : } else { : D > is this thread safe? Done 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>" > as these become more and more complex, it might be worth re-visiting the fo Sure, since the debug action in this patch is being passed in as a command line flag its a little awkward to make it JSON or Thrift but its worth thinking about. http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@100 PS2, Line 100: [@<error>] returns > this is optional right? should we document the behavior if it is omitted? Done http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py File tests/custom_cluster/test_rpc_exception.py: http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@27 PS2, Line 27: # DataStreamService rpc names : TRANSMIT_DATA_RPC = "TransmitData" : END_DATA_STREAM_RPC = "EndDataStream" : : # Error to specify for ImpalaServicePool to reject rpcs with a 'server too busy' error. : REJECT_TOO_BUSY_MSG = "REJECT_TOO_BUSY" : > can you document these all a bit? Done http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@76 PS2, Line 76: assert self._get_num_fails(impalad) > 0 : : def _get_fail_action(rpc, error=None, port=KRPC_PORT, p=0.1): > can you document this a bit more and explain what exactly this debug action Done http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88 PS2, Line 88: @CustomCluster > define this and END_ERROR at the top of the file, next to REJECT_TOO_BUSY_M Done http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88 PS2, Line 88: Suite.with_args("-- > is this and END_DATA_STREAM_ERROR suppose to be defined in this patch? 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: 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: Fri, 08 Nov 2019 23:58:37 +0000 Gerrit-HasComments: Yes
