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 <[email protected]>
Reviewed-by: Adar Dembo <[email protected]>
---
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 <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Todd Lipcon <[email protected]>
