[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation

2017-11-03 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8455 )

Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run 
after query failure/cancellation
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1130
PS1, Line 1130: swap(rpc_params.bloom_filter, aggregated_filter);
I think we should also track the rpc_params memory and prevent the query from 
being unregistered and releasing admission control resources until this 
function has exited - could you maybe file a follow-up JIRA? That might depend 
on my admission control change anyway.



--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 03 Nov 2017 16:08:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation

2017-11-03 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8455 )

Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run 
after query failure/cancellation
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc@394
PS1, Line 394:   if (!status_.ok()) return;
> same
Also, should we check if the backend is done too?

I think we should also include a comment explaining what this achieves.


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1106
PS1, Line 1106: if (state->disabled()) return;
I think checking for the filter disabled is sufficient in this function, since 
it's disabled when resources are released.



--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 03 Nov 2017 16:02:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation

2017-11-03 Thread Bharath Vissapragada (Code Review) 
Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8455 )

Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run 
after query failure/cancellation
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator-backend-state.cc@394
PS1, Line 394:   if (!status_.ok()) return;
same


http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

http://gerrit.cloudera.org:8080/#/c/8455/1/be/src/runtime/coordinator.cc@1082
PS1, Line 1082:   if (!query_status_.ok()) return;
lock_?



--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Fri, 03 Nov 2017 06:44:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query failure/cancellation

2017-11-02 Thread Sailesh Mukil (Code Review) 
Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8455


Change subject: IMPALA-6144: UpdateFilter()/PublishFilter() continue to run 
after query failure/cancellation
..

IMPALA-6144: UpdateFilter()/PublishFilter() continue to run after query 
failure/cancellation

The Coordinator::UpdateFilter() and Coordinator::PublishFilter()
functions do not check for query failure/cancellation. So if
runtime filters are being aggregated/published during a failure,
they will not be cancelled and still be sent out which may take
a while depending on the size of the cluster. Also, these functions
could potentially hold very large amounts of untracked memory.

This patch fixes it by checking for cancellation in both the
functions.

Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
2 files changed, 3 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/8455/1
--
To view, visit http://gerrit.cloudera.org:8080/8455
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I400456ad85adb9c23d2d432d772311fa4dcff2ed
Gerrit-Change-Number: 8455
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil