You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/03/21 23:23:08 UTC
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9747
to review the following change.
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
KUDU-2356. Idle WALs should not consume significant memory
Prior to this patch, each WAL writer would keep its temporary
compress_buf_ sized based on the largest data that was ever written to
it, even if the tablet was idle. Keeping the buffer around for a hot WAL
probably has some small performance benefit, but when the WAL is idle
enough to shut down its thread it should also clear its buffer to avoid
occupying memory.
Change-Id: I977976ef9505401b1bd596046915473a91153cd9
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
5 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/9747/1
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed a vote on this change.
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9747
to look at the new patch set (#3).
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
KUDU-2356. Idle WALs should not consume significant memory
Prior to this patch, each WAL writer would keep its temporary
compress_buf_ sized based on the largest data that was ever written to
it, even if the tablet was idle. Keeping the buffer around for a hot WAL
probably has some small performance benefit, but when the WAL is idle
enough to shut down its thread it should also clear its buffer to avoid
occupying memory.
Change-Id: I977976ef9505401b1bd596046915473a91153cd9
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
5 files changed, 23 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/9747/3
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9747 )
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
KUDU-2356. Idle WALs should not consume significant memory
Prior to this patch, each WAL writer would keep its temporary
compress_buf_ sized based on the largest data that was ever written to
it, even if the tablet was idle. Keeping the buffer around for a hot WAL
probably has some small performance benefit, but when the WAL is idle
enough to shut down its thread it should also clear its buffer to avoid
occupying memory.
Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Reviewed-on: http://gerrit.cloudera.org:8080/9747
Tested-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
5 files changed, 23 insertions(+), 0 deletions(-)
Approvals:
Todd Lipcon: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9747
to look at the new patch set (#2).
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
KUDU-2356. Idle WALs should not consume significant memory
Prior to this patch, each WAL writer would keep its temporary
compress_buf_ sized based on the largest data that was ever written to
it, even if the tablet was idle. Keeping the buffer around for a hot WAL
probably has some small performance benefit, but when the WAL is idle
enough to shut down its thread it should also clear its buffer to avoid
occupying memory.
Change-Id: I977976ef9505401b1bd596046915473a91153cd9
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
5 files changed, 22 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/9747/2
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9747 )
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
Patch Set 3: Verified+1
Hit the known unrelated DnsResolver race
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Mar 2018 02:48:39 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9747 )
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 21 Mar 2018 23:32:09 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9747 )
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9747/3/src/kudu/consensus/log-test.cc
File src/kudu/consensus/log-test.cc:
http://gerrit.cloudera.org:8080/#/c/9747/3/src/kudu/consensus/log-test.cc@1144
PS3, Line 1144: debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
> So I understand that these scope thingies will tell TSAN to back off, but a
I don't think so because this test appends such a small amount of data that the WAL would not be in danger of rolling, and thus it wouldn't switch segments.
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Mar 2018 16:39:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2356. Idle WALs should not consume significant memory
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9747 )
Change subject: KUDU-2356. Idle WALs should not consume significant memory
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9747/3/src/kudu/consensus/log-test.cc
File src/kudu/consensus/log-test.cc:
http://gerrit.cloudera.org:8080/#/c/9747/3/src/kudu/consensus/log-test.cc@1144
PS3, Line 1144: debug::ScopedTSANIgnoreReadsAndWrites ignore_tsan;
So I understand that these scope thingies will tell TSAN to back off, but are these read accesses actually safe? Is there any danger of log_->active_segment_ becoming null or something?
--
To view, visit http://gerrit.cloudera.org:8080/9747
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I977976ef9505401b1bd596046915473a91153cd9
Gerrit-Change-Number: 9747
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 22 Mar 2018 16:24:37 +0000
Gerrit-HasComments: Yes