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 <[email protected]> Reviewed-by: Adar Dembo <[email protected]> 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]>
