Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14641 )
Change subject: IMPALA-8138: Reintroduce rpc debugging options ...................................................................... Patch Set 2: (12 comments) First pass. Mostly clarification questions + requests for documentation. 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: RPC_SERVICE_POOL do you mean IMPALA_SERVICE_POOL? 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: string hostname_; : string port_; nit: use std::string 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: DebugAction(FLAGS_debug_actions, "IMPALA_SERVICE_POOL", : {hostname_, port_, c->remote_method().method_name()}) would be good to document this some more, it is basically a DebugAction that matches on IMPALA_SERVICE_POOL, the hostname, port, and the remote method name? what is the remote method name suppose to represent? 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? 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] : : Substitute("Debug Action: $0:$1", components[0], action_str); should returning tokens[2] only be done if the action is FAIL? might be good to document what error message is returned, either here or in the method declaration. http://gerrit.cloudera.org:8080/#/c/14641/2/be/src/util/debug-util.cc@399 PS2, Line 399: IntCounter* metric = : ExecEnv::GetInstance()->metrics()->FindMetricForTesting<IntCounter>( : "impala.debug_action.fail"); : if (!metric) { : metric = : ExecEnv::GetInstance()->metrics()->AddCounter("impala.debug_action.fail", 0); : } is this thread safe? 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 format we define debug actions in. probably out of scope for this JIRA, but might be worth filing a separate JIRA to revisit how debug actions are defined. maybe defining them as JSON or Thrift objects would be cleaner. http://gerrit.cloudera.org:8080/#/c/14641/2/common/thrift/ImpalaService.thrift@100 PS2, Line 100: [@<error message>] this is optional right? should we document the behavior if it is omitted? 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 : TRANSMIT_DATA_RPC = "TransmitData" : END_DATA_STREAM_RPC = "EndDataStream" : : REJECT_TOO_BUSY_MSG = "REJECT_TOO_BUSY" : : KRPC_PORT = 27002 can you document these all a bit? http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@76 PS2, Line 76: def _get_fail_action(rpc, error, p=0.1, port=KRPC_PORT): : return "IMPALA_SERVICE_POOL:127.0.0.1:{port}:{rpc}:FAIL@{probability}@{error}" \ : .format(rpc=rpc, error=error, probability=p, port=port) can you document this a bit more and explain what exactly this debug action is suppose to do. http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88 PS2, Line 88: TRANSMIT_ERROR define this and END_ERROR at the top of the file, next to REJECT_TOO_BUSY_MSG? would be good to document them all a bit as well http://gerrit.cloudera.org:8080/#/c/14641/2/tests/custom_cluster/test_rpc_exception.py@88 PS2, Line 88: TRANSMIT_DATA_ERROR is this and END_DATA_STREAM_ERROR suppose to be defined in this patch? -- 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: 2 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Thu, 07 Nov 2019 17:31:12 +0000 Gerrit-HasComments: Yes
