You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2018/06/25 23:27:46 UTC
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10815
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 34 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/1
--
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: newchange
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil 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:
(1 comment)
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@336
PS1, Line 336: const TQueryCtx& query_ctx
This unfortunately makes the code look a little less readable. Since we require the TQueryCtx in multiple places in BackendState, how about storing the TQueryCtx at BackendState object construction time?
Or alternatively, store a pointer to Coordinator, and get it from Coodrdinator::query_ctx() ?
--
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 <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Jun 2018 20:06:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10815
to look at the new patch set (#6).
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 51 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/6
--
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: newpatchset
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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()?
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 of the rpc?
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 and in a "|". You could do this instead:
debug_action = "|".join(filter(None, [wait_action, fail_rpc_action]))
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'..."
--
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 21:43:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/10815/2/be/src/runtime/coordinator-backend-state.h@262
PS2, Line 262:
I'll remove that whitespace before committing.
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@336
PS1, Line 336: ) {
> This unfortunately makes the code look a little less readable. Since we req
I'm curious why you feel this is less readable.
I tend to think that passing a parameter for dependencies is more readable for the following reason: As you know, impala has a history of having bugs due to object lifetime mismatch issues. These issues arose when objects (of differing lifetimes) were linked together via pointers. That creates implicit dependencies of lifetimes that is hard to reason about.
This can be avoided by expressing the dependencies of code explicitly by passing function arguments. It's obvious that the caller of these functions must pass a TQueryCtx with sufficient lifetime for this call.
We've also been systematically addressing the lifetime issue by simplifying and making consistent the lifetime of control structures. For the backend executor side, that means all query control structures are owned (and have the same lifetime as) the QueryState. For the coordinator side, we haven't gotten as far on the cleanup but we should (and it'd likely be the ClientRequestState lifetime, once we clean that class up).
The other problem with creating links between objects is that it makes it easy to be lazy about defining the right abstractions, which e.g. leads to bi-direction calls between objects (i.e spaghetti code).
Also, I don't want to copy the TQueryCtx into each BackendState because then it becomes less clear that this is a query-wide structure and the copies per backend are a waste of memory (though maybe you meant copy a pointer to it?).
All that said, given the tight relationship between this class and the Coordinator (and the fact that the Coordinator owns this), I'm okay with keeping a back pointer to the coordinator to access the query metadata. I think both patchsets are okay, but I assume you prefer the second. LMK if not.
--
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: 2
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 04:07:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10815
to look at the new patch set (#4).
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 49 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/4
--
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: newpatchset
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil 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 6: Code-Review+2
(1 comment)
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@336
PS1, Line 336: ) {
> I'm curious why you feel this is less readable.
As we spoke, my basic reasoning was that if we have a function signature like so:
def Func(param..)
then that would signify that 'param' is used to do 'Func'.
In this case:
bool Cancel(query_ctx)
that would signify, use 'query_ctx' to 'Cancel'.
However, 'query_ctx' is used for an orthogonal use case within the function (which is important, but is just not doing any real 'cancel' work).
There are cases where this pattern is unavoidable, but I feel like we could do without it here. The second patch set looks much better though. Thanks for doing that.
--
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: 6
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 02:58:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+1
Fix whitespace glitch, carry.
--
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: 6
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 23:39:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10815
to look at the new patch set (#3).
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 50 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/3
--
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: newpatchset
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7: Code-Review+2
--
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: 7
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 04:12:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10815
to look at the new patch set (#2).
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 54 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/2
--
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: newpatchset
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2752/ DRY_RUN=false
--
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: 7
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 04:12:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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:FAIL@0.3
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 22:24:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7: Verified+1
--
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: 7
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Jun 2018 07:38:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 5: Code-Review+1
(2 comments)
Carry Lars' +1
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:
> Thanks for the explanation. I think the code could be more readable if we r
Agree, done.
http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py@136
PS4, Line 136: "|".join(filter
> remove one "debug_action = "
Oops, 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: 5
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 23:37:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10815
to look at the new patch set (#5).
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 51 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/15/10815/5
--
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: newpatchset
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker 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 4: Code-Review+1
(3 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,
> No, I wanted to simulate what happens when a single RPC fails. That way we
Thanks for the explanation. I think the code could be more readable if we reduced the indent by using continue, both for !client_status.ok() above and for !rpc_status.ok() here. I don't feel that strongly about it though.
http://gerrit.cloudera.org:8080/#/c/10815/1/be/src/runtime/coordinator-backend-state.cc@367
PS1, Line 367: COORD_CANCEL_QUERY_FINSTANCES_RPC
> The convention I've been using for the global debug action labels is to pre
I agree, with the COORD prefix it actually is clear enough.
http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/10815/4/tests/query_test/test_cancellation.py@136
PS4, Line 136: debug_action =
remove one "debug_action = "
--
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: 4
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 23:06:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10815 )
Change subject: IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
......................................................................
IMPALA-7205: Respond to ReportExecStatus() RPC with CANCELLED if query execution has terminated
Otherwise, if the coordinator to backend CancelFInstances() RPC had failed,
the query can hang (and/or finstances can continue running until the
query is closed.
Testing:
- the modified test reproduces the hang without the impalad fix
Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Reviewed-on: http://gerrit.cloudera.org:8080/10815
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M tests/query_test/test_cancellation.py
4 files changed, 51 insertions(+), 38 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I7bb2c26edace89853f14a329f891d1f9a065a991
Gerrit-Change-Number: 10815
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>