You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2019/10/26 07:32:55 UTC
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14554
to review the following change.
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Revert "Revert "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.
Revert notes:
The original patch was reverted because there were TSAN failures in this
new behavior. This was caused by the thread-unsafe intermingling of the
append thread going idle (and thus the active segment going idle) and a
separate thread calling AllocateSegmentAndRollOver() (a method used in test
only), which would swap out a new active segment.
The race can be illustrated with the following sequence of events, with
the append thread (P), allocation thread (L), and test thread (T).
T: writes a batch to the log asynchronously, waking up the append thread
P: handles the batch
P: after a while, there's no work to be done
P: sets the thread state to IDLE
T: calls AllocateSegmentAndRollover()
L: asynchronously allocates a new segment file
T: synchronously waits for the allocation to finish
T: sets the active segment to the newly allocated segment
P: simultaneously calls active_segment->GoIdle()
The last two steps here are what race with each other. The original
change assumed that the append thread was the sole thread operating on
the active segment, which in production is true, but in some tests, is
not the case.
Since the race only occurs 1) when the active segment is going idle, and
2) a separate thread is trying to switch active segments, I've added a
lock to protect the active segment going idle. There may be a more
elegant way around this that's less ad-hoc, but the new lock is the
smallest amount of concurrency control I could think of to fix the race.
Without the new lock, tablet_copy_client-test would fail 16/100 times in
TSAN mode. With it, it passed 100/100 times.
Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
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
M src/kudu/tserver/tablet_copy-test-base.h
6 files changed, 47 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14554/1
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Revert "Revert "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/14554 )
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Patch Set 4: Code-Review+2
Normally I'd ask whether we can stop depending on an unsafe method like AllocateSegmentAndRollover (especially since the production path is so different), but that looks like it'd add more complexity than it's worth.
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 28 Oct 2019 04:12:19 +0000
Gerrit-HasComments: No
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14554 )
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Revert "Revert "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.
Revert notes:
The original patch was reverted because there were TSAN failures in this
new behavior. This was caused by the thread-unsafe intermingling of the
append thread going idle (and thus the active segment going idle) and a
separate thread calling AllocateSegmentAndRollOver() (a method used in test
only), which would swap out a new active segment.
The race can be illustrated with the following sequence of events, with
the append thread (P), allocation thread (L), and test thread (T).
T: writes a batch to the log asynchronously, waking up the append thread
P: handles the batch
P: after a while, there's no work to be done
P: sets the thread state to IDLE
T: calls AllocateSegmentAndRollover()
L: asynchronously allocates a new segment file
T: synchronously waits for the allocation to finish
T: sets the active segment to the newly allocated segment
P: simultaneously calls active_segment->GoIdle()
The last two steps here are what race with each other. The original
change assumed that the append thread was the sole thread operating on
the active segment, which in production is true, but in some tests, is
not the case.
Since the race only occurs 1) when the active segment is going idle, and
2) a separate thread is trying to switch active segments, I've added a
lock to protect the active segment going idle. There may be a more
elegant way around this that's less ad-hoc, but the new lock is the
smallest amount of concurrency control I could think of to fix the race.
AllocateSegmentAndRollover() is used in tablet_copy-test-base.h and
log-test-base.h. Without the new lock, tablet_copy_client-test would
fail 16/100 times in TSAN mode. With it, it passed 300/300 times.
log-test.h previously failed 106/300 times without it, and passed
300/300 with it.
Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Reviewed-on: http://gerrit.cloudera.org:8080/9747
Tested-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-on: http://gerrit.cloudera.org:8080/14554
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/log-test-base.h
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
6 files changed, 40 insertions(+), 6 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14554
to look at the new patch set (#2).
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Revert "Revert "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.
Revert notes:
The original patch was reverted because there were TSAN failures in this
new behavior. This was caused by the thread-unsafe intermingling of the
append thread going idle (and thus the active segment going idle) and a
separate thread calling AllocateSegmentAndRollOver() (a method used in test
only), which would swap out a new active segment.
The race can be illustrated with the following sequence of events, with
the append thread (P), allocation thread (L), and test thread (T).
T: writes a batch to the log asynchronously, waking up the append thread
P: handles the batch
P: after a while, there's no work to be done
P: sets the thread state to IDLE
T: calls AllocateSegmentAndRollover()
L: asynchronously allocates a new segment file
T: synchronously waits for the allocation to finish
T: sets the active segment to the newly allocated segment
P: simultaneously calls active_segment->GoIdle()
The last two steps here are what race with each other. The original
change assumed that the append thread was the sole thread operating on
the active segment, which in production is true, but in some tests, is
not the case.
Since the race only occurs 1) when the active segment is going idle, and
2) a separate thread is trying to switch active segments, I've added a
lock to protect the active segment going idle. There may be a more
elegant way around this that's less ad-hoc, but the new lock is the
smallest amount of concurrency control I could think of to fix the race.
Without the new lock, tablet_copy_client-test would fail 16/100 times in
TSAN mode. With it, it passed 300/300 times.
Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
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
M src/kudu/tserver/tablet_copy-test-base.h
6 files changed, 41 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14554/2
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14554
to look at the new patch set (#4).
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Revert "Revert "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.
Revert notes:
The original patch was reverted because there were TSAN failures in this
new behavior. This was caused by the thread-unsafe intermingling of the
append thread going idle (and thus the active segment going idle) and a
separate thread calling AllocateSegmentAndRollOver() (a method used in test
only), which would swap out a new active segment.
The race can be illustrated with the following sequence of events, with
the append thread (P), allocation thread (L), and test thread (T).
T: writes a batch to the log asynchronously, waking up the append thread
P: handles the batch
P: after a while, there's no work to be done
P: sets the thread state to IDLE
T: calls AllocateSegmentAndRollover()
L: asynchronously allocates a new segment file
T: synchronously waits for the allocation to finish
T: sets the active segment to the newly allocated segment
P: simultaneously calls active_segment->GoIdle()
The last two steps here are what race with each other. The original
change assumed that the append thread was the sole thread operating on
the active segment, which in production is true, but in some tests, is
not the case.
Since the race only occurs 1) when the active segment is going idle, and
2) a separate thread is trying to switch active segments, I've added a
lock to protect the active segment going idle. There may be a more
elegant way around this that's less ad-hoc, but the new lock is the
smallest amount of concurrency control I could think of to fix the race.
AllocateSegmentAndRollover() is used in tablet_copy-test-base.h and
log-test-base.h. Without the new lock, tablet_copy_client-test would
fail 16/100 times in TSAN mode. With it, it passed 300/300 times.
log-test.h previously failed 106/300 times without it, and passed
300/300 with it.
Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
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-base.h
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
6 files changed, 40 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14554/4
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14554 )
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Patch Set 4:
> Patch Set 4: Code-Review+2
>
> Normally I'd ask whether we can stop depending on an unsafe method like AllocateSegmentAndRollover (especially since the production path is so different), but that looks like it'd add more complexity than it's worth.
Yeah, it's intertwined in quite a few tests unfortunately. Maybe it'll be easier to do that after my refactoring patch lands, but for now, I wouldn't gate this bugfix on it.
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 28 Oct 2019 04:24:47 +0000
Gerrit-HasComments: No
[kudu-CR] Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14554
to look at the new patch set (#3).
Change subject: Revert "Revert "KUDU-2356. Idle WALs should not consume significant memory""
......................................................................
Revert "Revert "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.
Revert notes:
The original patch was reverted because there were TSAN failures in this
new behavior. This was caused by the thread-unsafe intermingling of the
append thread going idle (and thus the active segment going idle) and a
separate thread calling AllocateSegmentAndRollOver() (a method used in test
only), which would swap out a new active segment.
The race can be illustrated with the following sequence of events, with
the append thread (P), allocation thread (L), and test thread (T).
T: writes a batch to the log asynchronously, waking up the append thread
P: handles the batch
P: after a while, there's no work to be done
P: sets the thread state to IDLE
T: calls AllocateSegmentAndRollover()
L: asynchronously allocates a new segment file
T: synchronously waits for the allocation to finish
T: sets the active segment to the newly allocated segment
P: simultaneously calls active_segment->GoIdle()
The last two steps here are what race with each other. The original
change assumed that the append thread was the sole thread operating on
the active segment, which in production is true, but in some tests, is
not the case.
Since the race only occurs 1) when the active segment is going idle, and
2) a separate thread is trying to switch active segments, I've added a
lock to protect the active segment going idle. There may be a more
elegant way around this that's less ad-hoc, but the new lock is the
smallest amount of concurrency control I could think of to fix the race.
Without the new lock, tablet_copy_client-test would fail 16/100 times in
TSAN mode. With it, it passed 300/300 times.
Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
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, 39 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/14554/3
--
To view, visit http://gerrit.cloudera.org:8080/14554
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I571a7cb3d310687e6e22bbd547e51a2ea81b8806
Gerrit-Change-Number: 14554
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>