You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Xianqing He (Code Review)" <ge...@cloudera.org> on 2021/07/29 10:08:40 UTC

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Xianqing He has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17735


Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................

IMPALA-10414: fix memory leak when cancel the retried query

The QueryDriver#retry_query_thread_ may not finish when cancel the
retried query. So the reference count of QueryDriver
(via the shared_ptr) will not go to 0 and the memory will leak.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
---
M be/src/runtime/query-driver.cc
M tests/custom_cluster/test_query_retries.py
2 files changed, 21 insertions(+), 0 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/17735/2
-- 
To view, visit http://gerrit.cloudera.org:8080/17735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 2
Gerrit-Owner: Xianqing He <he...@126.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
Thanks for catching this issue. This Finalize() call seems missing. One question. ClientRequestState::Finalize() will call ClientRequestState::Cancel() for request_state again since the Cancel() function is already called in line 241 in this function. But ClientRequestState::Cancel() does not check if member variable is_cancelled_ is already true. I think ClientRequestState::Finalize() should check is_cancelled_ before call Cancel(), or ClientRequestState::Cancel() check is_cancelled_ in the beginning to avoid calling cancellation for coordinator and admission control again.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Sat, 09 Apr 2022 01:27:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Apr 2022 21:07:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8:

> Patch Set 8:
> 
> > Patch Set 8: Code-Review+1
> > 
> > So sorry to be late here.. I triggered two exhaustive builds:
> > https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16239/ (DEBUG)
> > https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16240/ (ASAN)
> > 
> > I think we can merge this after they pass.
> 
> Hi Xianqing,
> The two jobs failed. Could you have a look on them? I haven't digged into them. Not sure if they are unrelated failures.

I think they are unrelated failures and need to rebase the code. The failures were fixed in IMPALA-10961, IMPALA-11196 and IMPALA-11109.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Mon, 11 Apr 2022 02:56:42 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
> Without calling ClientRequestState::Finalize(), the transaction, which is o
Different places call ClientRequestState::Cancel() with different parameters, I'm not sure whether we will hit some illegal states when checking is_cancelled_ in the beginning of ClientRequestState::Cancel().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Mon, 11 Apr 2022 03:59:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10: Code-Review+2

Thanks for your review, Wenzhe!
and thank Xianqing's hard work!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Fri, 15 Apr 2022 01:36:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
> We can add a new API in Coordinator to wait until finalized. In ClientReque
Read cancellation functions. ChildQuery::Cancel() and Coordinator::Cancel() are safe to be recalled. But  RemoteAdmissionControlClient::CancelAdmission() could send extra RPC if it's recalled. We can make simple change for ClientRequestState::Cancel(), don't call admission_control_client_->CancelAdmission() if is_cancelled_ is already true.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Wed, 13 Apr 2022 06:58:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................

IMPALA-10414: fix memory leak when cancel the retried query

The QueryDriver#retry_query_thread_ may not finish when cancel the
retried query. So the reference count of QueryDriver
(via the shared_ptr) will not go to 0 and the memory will leak.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
---
M be/src/runtime/query-driver.cc
M tests/custom_cluster/test_query_retries.py
2 files changed, 22 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/17735/5
-- 
To view, visit http://gerrit.cloudera.org:8080/17735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 5
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
> Agree. When ClientRequestState::Cancel() is called with wait_until_finalize
We can add a new API in Coordinator to wait until finalized. In ClientRequestState::Cancel(), we can call this new API instead of Coordinator::Cancel() if both parameter wait_until_finalized and is_cancelled_ are true.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Tue, 12 Apr 2022 05:52:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
> Different places call ClientRequestState::Cancel() with different parameter
Agree. When ClientRequestState::Cancel() is called with wait_until_finalized as true, we have to wait until finalized.  Query retry make the cancellation code messy. I recently saw a case for which Coordinator::BackendState::Cancel() was hung and it caused admission resource not been released. I think it's not safe to call ClientRequestState::Cancel() more than once since it call AdmissionControlService::CancelAdmission() and Coordinator::Cancel().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Mon, 11 Apr 2022 05:24:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................

IMPALA-10414: fix memory leak when canceling the retried query

The query retry launches in a separate thread. This thread may not
finishes when deleting the query from the given QueryDriverMap if
the query retry was failed launched. In this case, the resources
for the query retry thread will not release. So the reference
count of QueryDriver (via the shared_ptr) will not go to 0 and it
will not be destroyed.

We need wait until the query retry thread execution has completed
when deleting the query from the given QueryDiverMap.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
---
M be/src/runtime/query-driver.cc
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_query_retries.py
3 files changed, 37 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/17735/10
-- 
To view, visit http://gerrit.cloudera.org:8080/17735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10451/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Apr 2022 11:18:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Fri, 11 Feb 2022 17:00:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/9212/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 5
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 30 Jul 2021 03:22:33 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Apr 2022 16:38:35 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/9205/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 3
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 10:36:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................


Patch Set 5:

(5 comments)

Thanks for uploading the fix! I'm still revisiting the cancellation and unregisteration codes. Post some questions for discussion first.

http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG@7
PS5, Line 7: cancel
nit: cancelling


http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG@11
PS5, Line 11: and the memory will leak
Could you add more explanation about this?


http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc@357
PS5, Line 357:   RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
Do we only need this in the corner case or we need it in all cases? Could you add a comment about this?


http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc@419
PS5, Line 419: Wait retry_query_thread_ finished
nit: Wait until retry_query_thread_ finishes


http://gerrit.cloudera.org:8080/#/c/17735/5/tests/custom_cluster/test_query_retries.py
File tests/custom_cluster/test_query_retries.py:

http://gerrit.cloudera.org:8080/#/c/17735/5/tests/custom_cluster/test_query_retries.py@1098
PS5, Line 1098: __validate_memz
Can we validate this at the end of all tests?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 5
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Fri, 06 Aug 2021 09:58:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8048/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Apr 2022 16:39:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7834/ DRY_RUN=true


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Fri, 11 Feb 2022 10:29:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8:

> Patch Set 8: Code-Review+1
> 
> So sorry to be late here.. I triggered two exhaustive builds:
> https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16239/ (DEBUG)
> https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16240/ (ASAN)
> 
> I think we can merge this after they pass.

Hi Xianqing,
The two jobs failed. Could you have a look on them? I haven't digged into them. Not sure if they are unrelated failures.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Sat, 09 Apr 2022 13:38:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................

IMPALA-10414: fix memory leak when canceling the retried query

The query retry launches in a separate thread. This thread may not
finishes when deleting the query from the given QueryDriverMap if
the query retry was failed launched. In this case, the resources
for the query retry thread will not release. So the reference
count of QueryDriver (via the shared_ptr) will not go to 0 and it
will not be destroyed.

We need wait until the query retry thread execution has completed
when deleting the query from the given QueryDiverMap.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Reviewed-on: http://gerrit.cloudera.org:8080/17735
Reviewed-by: Wenzhe Zhou <wz...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Quanlong Huang <hu...@gmail.com>
---
M be/src/runtime/query-driver.cc
M be/src/service/client-request-state.cc
M tests/custom_cluster/test_query_retries.py
3 files changed, 37 insertions(+), 2 deletions(-)

Approvals:
  Wenzhe Zhou: Looks good to me, but someone else must approve
  Impala Public Jenkins: Verified
  Quanlong Huang: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 11
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG@7
PS5, Line 7: cancel
> nit: cancelling
Done


http://gerrit.cloudera.org:8080/#/c/17735/5//COMMIT_MSG@11
PS5, Line 11: n this case, the resourc
> Could you add more explanation about this?
Done


http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc@357
PS5, Line 357:   request_state->MarkAsRetried(retry_query_id);
> Do we only need this in the corner case or we need it in all cases? Could y
Done


http://gerrit.cloudera.org:8080/#/c/17735/5/be/src/runtime/query-driver.cc@419
PS5, Line 419: ueryHandle* query_handle, bool ch
> nit: Wait until retry_query_thread_ finishes
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 7
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Oct 2021 08:47:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................

IMPALA-10414: fix memory leak when cancel the retried query

The QueryDriver#retry_query_thread_ may not finish when cancel the
retried query. So the reference count of QueryDriver
(via the shared_ptr) will not go to 0 and the memory will leak.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
---
M be/src/runtime/query-driver.cc
M tests/custom_cluster/test_query_retries.py
2 files changed, 22 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/17735/3
-- 
To view, visit http://gerrit.cloudera.org:8080/17735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 3
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/9204/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 2
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 10:31:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc
File be/src/runtime/query-driver.cc:

http://gerrit.cloudera.org:8080/#/c/17735/8/be/src/runtime/query-driver.cc@368
PS8, Line 368: RETURN_VOID_IF_ERROR(query_handle->Finalize(true, nullptr));
> Thanks for catching this issue. This Finalize() call seems missing. One que
Without calling ClientRequestState::Finalize(), the transaction, which is opened by original query, will not be closed. This is another way to validate ClientRequestState::Finalize() is missing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Sat, 09 Apr 2022 03:18:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 8: Code-Review+1

So sorry to be late here.. I triggered two exhaustive builds:
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16239/ (DEBUG)
https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/16240/ (ASAN)

I think we can merge this after they pass.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 8
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Fri, 08 Apr 2022 06:50:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................

IMPALA-10414: fix memory leak when canceling the retried query

The query retry launches in a separate thread. This thread may not
finishes when deleting the query from the given QueryDriverMap if
the query retry was failed launched. In this case, the resources
for the query retry thread will not release. So the reference
count of QueryDriver (via the shared_ptr) will not go to 0 and it
will not be destroyed.

We need wait until the query retry thread execution has completed
when deleting the query from the given QueryDiverMap.

Testing:
Modify the test_query_retries.py to verify memory leak by checking
the debug web UI of memz.

Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
---
M be/src/runtime/query-driver.cc
M tests/custom_cluster/test_query_retries.py
2 files changed, 31 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/17735/7
-- 
To view, visit http://gerrit.cloudera.org:8080/17735
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 7
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>

[Impala-ASF-CR] IMPALA-10414: fix memory leak when cancel the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when cancel the retried query
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/17735/2/tests/custom_cluster/test_query_retries.py
File tests/custom_cluster/test_query_retries.py:

http://gerrit.cloudera.org:8080/#/c/17735/2/tests/custom_cluster/test_query_retries.py@1093
PS2, Line 1093: '
flake8: E501 line too long (104 > 90 characters)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 2
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 29 Jul 2021 10:09:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/9606/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 7
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Thu, 14 Oct 2021 08:47:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Xianqing He (Code Review)" <ge...@cloudera.org>.
Xianqing He has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 10:

> Patch Set 9:
> 
> Ran debug exhaustive test and asan test for this patch, plus following minor changes for ClientRequestState::Cancel()
> 
> -    admission_control_client_->CancelAdmission();
> -    is_cancelled_ = true;
> +    if (!is_cancelled_) {
> +      admission_control_client_->CancelAdmission();
> +      is_cancelled_ = true;
> +    }
>    } // Release lock_ before doing cancellation work.
> 
> Passed debug exhaustive test without error
> (https://jenkins.impala.io/job/pre-review-test/1323/)
> Got one failure (TestAcid.test_lock_timings) for asan test.
> (https://master-03.jenkins.cloudera.com/job/impala-private-parameterized/915/). I think this failure is unrelavent.

Thanks for testing it. I had fixed as this.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 10
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Thu, 14 Apr 2022 11:00:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-10414: fix memory leak when canceling the retried query

Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/17735 )

Change subject: IMPALA-10414: fix memory leak when canceling the retried query
......................................................................


Patch Set 9:

Ran debug exhaustive test and asan test for this patch, plus following minor changes for ClientRequestState::Cancel()

-    admission_control_client_->CancelAdmission();
-    is_cancelled_ = true;
+    if (!is_cancelled_) {
+      admission_control_client_->CancelAdmission();
+      is_cancelled_ = true;
+    }
   } // Release lock_ before doing cancellation work.

Passed debug exhaustive test without error
(https://jenkins.impala.io/job/pre-review-test/1323/)
Got one failure (TestAcid.test_lock_timings) for asan test.
(https://master-03.jenkins.cloudera.com/job/impala-private-parameterized/915/). I think this failure is unrelavent.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If804ca65da1794c819a6b2e6567ea7651ab5112f
Gerrit-Change-Number: 17735
Gerrit-PatchSet: 9
Gerrit-Owner: Xianqing He <he...@126.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Xianqing He <he...@126.com>
Gerrit-Comment-Date: Wed, 13 Apr 2022 22:41:56 +0000
Gerrit-HasComments: No