Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10815 )
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@366 PS1, Line 366: rpc_status = DebugAction(query_ctx.client_request.query_options, > Wouldn't we need to break; the loop if this was not ok()? No, I wanted to simulate what happens when a single RPC fails. That way we can even exercise the retry logic with e.g. COORD_CANCEL_QUERY_FINSTANCES_RPC:[email protected] In the test for failing the RPC altogether, I give it a failure probability of 100%, so it loops 3 times and then gives up, just like what would happen if the RPC really failed. http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@367 PS1, Line 367: COORD_CANCEL_QUERY_FINSTANCES_RPC > Should we name this actions so that it's clear that it's in the sender side The convention I've been using for the global debug action labels is to prefix with the containing module name. So, I think the "COORD" prefix is sufficient for knowing this is the sender side. If we want to add one to the handler, it'd probably be called QUERYSTATE_CANCEL_QUERY_FINSTANCES_RPC. LMK if you feel it's still not clear. http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py@139 PS1, Line 139: if fail_rpc_action != None: > If wait_action != None and fail_rpc_action == None, then debug_action will Sure, if you feel that's more readable to a native python speaker. (I'd have to try it out to know what it's doing but I don't consider myself the most proficient python coder). Note that the extraneous "|" was harmless, but happy to change this, it does seem more python-y http://gerrit.cloudera.org:8080/#/c/10815/1/tests/query_test/test_cancellation.py@206 PS1, Line 206: if wait_action is None and fail_rpc_action is None \ > Then this could be "if not debug_action and ('count'..." Done -- To view, visit http://gerrit.cloudera.org:8080/10815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991 Gerrit-Change-Number: 10815 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Wed, 27 Jun 2018 22:24:01 +0000 Gerrit-HasComments: Yes
