You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/07/31 06:39:51 UTC
[kudu-CR] [tests] fix potential memory leak
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16265
Change subject: [tests] fix potential memory leak
......................................................................
[tests] fix potential memory leak
Fixed memory leak in kudu-tool-test when running calling RunLoadgen()
multiple times in a test scenario. For details, see
https://gerrit.cloudera.org/#/c/16253/4/src/kudu/tools/kudu-tool-test.cc@2281
Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/tools/kudu-tool-test.cc
2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/65/16265/1
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [tests] fix potential memory leak
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc@913
PS1, Line 913: STLDeleteValues(&ts_map_);
nit: would it be possible to put this into CreateTabletServerMap() instead, so callers don't have to worry about this?
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jul 2020 17:45:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix potential memory leak
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc@913
PS1, Line 913: STLDeleteValues(&ts_map_);
> Thank you for explaining. Good point about how it would leave this with con
Thank you for fast review!
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jul 2020 20:05:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix potential memory leak
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [tests] fix potential memory leak
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tests] fix potential memory leak
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc@913
PS1, Line 913: STLDeleteValues(&ts_map_);
> nit: would it be possible to put this into CreateTabletServerMap() instead,
I intentionally didn't put that into CreateTabletServerMap(), and the reason is CreateTabletServerMap()'s signature. The third parameter of the function allows for passing in a container that contains pointers to TServerDetails which are referred from other unordered_map<string, TServerDetails*> container, and those might be de-allocated at the upper level as well. That would be double-free error. Instead, I removed ts_map->clear() and added CHECK(ts_map->empty()) into CreateTabletServerMap() to track programming errors associated with the current clumsy signature and semantics of CreateTabletServerMap() function.
Overall, I think unordered_map<string, TServerDetails*> should be replaced by unordered_map<string, unique_ptr<TServerDetails>> or unordered_map<string, shared_ptr<TServerDetails>>, but I don't want to do this right now because it's a lot of yak shaving.
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jul 2020 18:04:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix potential memory leak
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16265/1/src/kudu/tools/kudu-tool-test.cc@913
PS1, Line 913: STLDeleteValues(&ts_map_);
> I intentionally didn't put that into CreateTabletServerMap(), and the reaso
Thank you for explaining. Good point about how it would leave this with convoluted memory-tracking semantics. I agree a smart pointer would be a better fit but +2 to deferring that in favor of this for now.
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jul 2020 18:40:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [tests] fix potential memory leak
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
[tests] fix potential memory leak
Fixed memory leak in kudu-tool-test when running calling RunLoadgen()
multiple times in a test scenario. For details, see
https://gerrit.cloudera.org/#/c/16253/4/src/kudu/tools/kudu-tool-test.cc@2281
Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Reviewed-on: http://gerrit.cloudera.org:8080/16265
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/tools/kudu-tool-test.cc
2 files changed, 2 insertions(+), 1 deletion(-)
Approvals:
Alexey Serbin: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tests] fix potential memory leak
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16265 )
Change subject: [tests] fix potential memory leak
......................................................................
Patch Set 1: Verified+1
unrelated test failure (RELEASE) in RangerClientTestBase.TestLogging
--
To view, visit http://gerrit.cloudera.org:8080/16265
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2e6132eb4cb027055b1523ff4e89d206d7eb973
Gerrit-Change-Number: 16265
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jul 2020 18:04:47 +0000
Gerrit-HasComments: No