You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yida Wu (Code Review)" <ge...@cloudera.org> on 2022/03/11 21:20:06 UTC

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18314


Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................

IMPALA-11176: Fix Memory leak in ClientCacheHelper

The patch fixes a memory leak issue in client cache.

The memory leak happens because a share pointer of the
ThriftClientImpl is created from a pointer of a base class while
the object is actually created as a derived class and forced
converted to the base. As a result, the shared pointer doesn't
know it is a derived object and could not release all the
resources the object holds.

The way to fix in the patch is in ClientCache::MakeClient, we
return the base shared pointer created by the pointer of the
derived class instead of returning a ThriftClientImpl pointer.

Added testcase ClientCacheTest.

Tests:
Ran exhaustive tests.

Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/client-cache-test.cc
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
4 files changed, 124 insertions(+), 7 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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/18314 )

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................

IMPALA-11176: Fix Memory leak in ClientCacheHelper

The patch fixes a memory leak issue in client cache.

The memory leak happens because a shared pointer of the
base class "ThriftClientImpl" is created from a derived class
"ThriftClient" while the base class's destructor is non-virtual.
As a result, the shared pointer can't locate the correct
destructor, which should be the derived one, and leads to
memory leaks.

The way to fix in the patch is to make the destructor of
"ThriftClientImpl" virtual. Added testcase ClientCacheTest.

Tests:
Ran exhaustive tests.

Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Reviewed-on: http://gerrit.cloudera.org:8080/18314
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/thrift-client.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/client-cache-test.cc
M be/src/runtime/client-cache.h
4 files changed, 118 insertions(+), 3 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10287/ : 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/18314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 12 Mar 2022 04:25:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

Thanks for adding the test! The fix LGTM. Just have few questions.

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/CMakeLists.txt
File be/src/runtime/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/CMakeLists.txt@117
PS3, Line 117: ADD_UNIFIED_BE_LSAN_TEST(client-cache-test ClientCacheTest.*)
This enables leak sanitizer in the test. So can we skip checking the virtual memory size? Or does leak sanitizer fail to detect the leak?


http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc
File be/src/runtime/client-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc@72
PS3, Line 72:     const int vm_size_pos = 22;
Will this index be consistent across all linux distros?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 11:47:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10284/ : 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/18314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 11 Mar 2022 21:39:55 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/CMakeLists.txt
File be/src/runtime/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/CMakeLists.txt@117
PS3, Line 117: ADD_UNIFIED_BE_LSAN_TEST(client-cache-test ClientCacheTest.*)
> This enables leak sanitizer in the test. So can we skip checking the virtua
Thanks for the advice. Tried the ASAN build, but failed to detect the leak. Maybe we need a newer compiler with newer ASAN


http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc
File be/src/runtime/client-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc@72
PS3, Line 72:     const int vm_size_pos = 22;
> Will this index be consistent across all linux distros?
The index comes with the linux kernel, and I traced the file "https://github.com/torvalds/linux/blob/v3.4/fs/proc/array.c" back to v3.4 2012, the index of vm size hasn't been changed since then. Maybe I can leave a comment in case the index might be changed and affect the test in the future.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 18:16:58 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 19:35:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG@11
PS2, Line 11: The memory leak happens because a shared pointer of the
> nit: 'shared'
Done


http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG@18
PS2, Line 18: The way to fix in the patch is to make the destructor of
            : "ThriftClientImpl" virtual. Added testcase ClientCacheTest.
            : 
            : Tests:
            : Ran exhaustive tests.
            : 
> We could probably remove this paragraph? Or maybe simply add "The fix is to
Done


http://gerrit.cloudera.org:8080/#/c/18314/2/be/src/runtime/client-cache-test.cc
File be/src/runtime/client-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/18314/2/be/src/runtime/client-cache-test.cc@70
PS2, Line 70:   uint64_t GetProcessVMSize() {
> Would have been ideal to get memory usage using memory pools and mem tracke
Yes, the testcase is using the exact same way as how the memory was leaking in our system. Tried the test in the old code, and it couldn't pass.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 07:28:06 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 14:35:39 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................

IMPALA-11176: Fix Memory leak in ClientCacheHelper

The patch fixes a memory leak issue in client cache.

The memory leak happens because a shared pointer of the
base class "ThriftClientImpl" is created from a derived class
"ThriftClient" while the base class's destructor is non-virtual.
As a result, the shared pointer can't locate the correct
destructor, which should be the derived one, and leads to
memory leaks.

The way to fix in the patch is to make the destructor of
"ThriftClientImpl" virtual. Added testcase ClientCacheTest.

Tests:
Ran exhaustive tests.

Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
---
M be/src/rpc/thrift-client.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/client-cache-test.cc
M be/src/runtime/client-cache.h
4 files changed, 118 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 2: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG@11
PS2, Line 11: The memory leak happens because a share pointer of the
nit: 'shared'


http://gerrit.cloudera.org:8080/#/c/18314/2//COMMIT_MSG@18
PS2, Line 18: The way to fix in the patch is to make the destructor of
            : "ThriftClientImpl" virtual and implement the destructor of the
            : derived class "ThriftClient". Therefore when a shared pointer
            : of the base class "ThriftClientImpl" is deleted that points
            : to the derived class "ThriftClient", the derived class's
            : destructor is called instead of the base's one.
We could probably remove this paragraph? Or maybe simply add "The fix is to make ThriftClientImpl's destructor virtual"


http://gerrit.cloudera.org:8080/#/c/18314/2/be/src/runtime/client-cache-test.cc
File be/src/runtime/client-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/18314/2/be/src/runtime/client-cache-test.cc@70
PS2, Line 70:   uint64_t GetProcessVMSize() {
Would have been ideal to get memory usage using memory pools and mem trackers. But, I think it will require lots of instrumentation, so this is probably okay for now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 02:59:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3:

(1 comment)

I can bump my +1 to +2 if no other reviewers want to double-check this.

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc
File be/src/runtime/client-cache-test.cc:

http://gerrit.cloudera.org:8080/#/c/18314/3/be/src/runtime/client-cache-test.cc@72
PS3, Line 72:     const int vm_size_pos = 22;
> The index comes with the linux kernel, and I traced the file "https://githu
Thanks for digging into this!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 15 Mar 2022 12:01:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3: Code-Review+2

We can check offline why ASAN doesn't find this leak. Would probably be useful for leak detection in general.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 15 Mar 2022 23:50:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/18314 )

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................

IMPALA-11176: Fix Memory leak in ClientCacheHelper

The patch fixes a memory leak issue in client cache.

The memory leak happens because a share pointer of the
base class "ThriftClientImpl" is created from a derived class
"ThriftClient" while the base class's destructor is non-virtual.
As a result, the shared pointer can't locate the correct
destructor, which should be the derived one, and leads to
memory leaks.

The way to fix in the patch is to make the destructor of
"ThriftClientImpl" virtual and implement the destructor of the
derived class "ThriftClient". Therefore when a shared pointer
of the base class "ThriftClientImpl" is deleted that points
to the derived class "ThriftClient", the derived class's
destructor is called instead of the base's one.

Added testcase ClientCacheTest.

Tests:
Ran exhaustive tests.

Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
---
M be/src/rpc/thrift-client.h
M be/src/runtime/CMakeLists.txt
A be/src/runtime/client-cache-test.cc
M be/src/runtime/client-cache.h
4 files changed, 118 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3:

Thanks Abhishek and Quanlong for the review. Will take a look at how to detect this type of leak.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 07:37:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 14:35:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11176: Fix Memory leak in ClientCacheHelper

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

Change subject: IMPALA-11176: Fix Memory leak in ClientCacheHelper
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/10289/ : 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/18314
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf3d7e7d897c89eb4538df5382ce3c590055b71
Gerrit-Change-Number: 18314
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 14 Mar 2022 07:48:29 +0000
Gerrit-HasComments: No