Move pipeline write waiting logic into WaitForPendingWrites() #5716
Conversation
|
CC @yiwu-arbug |
|
How about re-enable pipeline writes like PR #5636 did and run TSAN to make sure it's now thread safe? |
|
LGTM overall, but don't quite understand what's the issue with flush scheduler. I think it is thread-safe by itself? |
|
@yiwu-arbug multiple threads calling ScheduleFlush() is thread-safe, but TakeNextColumnFamily() is not. |
|
@miasantreble I did run tsan on my dev box and it passed. Let me turn it on in crash test. |
|
@yiwu-arbug it's also that, getting list of CFs whose memtable is full before waiting is less straight-forward to reason about. It means that, after waiting for the memtable writes finish, we might leave some full memtables unflushed, and restart the pipeline again. It might not be necessarily wrong, but harder to prove. |
One issue I can see is |
agree. |
|
ping @maysamyabandeh |
|
@yiwu-arbug if it looks good to you, can you approve the PR? |
maysamyabandeh
requested review from
yiwu-arbug
and removed request for
maysamyabandeh
Summary: In pipeline writing mode, memtable switching needs to wait for memtable writing to finish to make sure that when memtables are made immutable, inserts are not going to them. This is currently done in DBImpl::SwitchMemtable(). This is done after flush_scheduler_.TakeNextColumnFamily() is called to fetch the list of column families to switch. The function flush_scheduler_.TakeNextColumnFamily() itself, however, is not thread-safe when being called together with flush_scheduler_.ScheduleFlush(). This change provides a fix, which moves the waiting logic before flush_scheduler_.TakeNextColumnFamily(). WaitForPendingWrites() is a natural place where the logic can happen. Test Plan: Run all tests with ASAN and TSAN.
siying
force-pushed
the
pipeline_wait_write
branch
from
d6c1ad9 to
13f7391
Compare
There was a problem hiding this comment.
@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in a3960fc. |
…ok#5716) Summary: In pipeline writing mode, memtable switching needs to wait for memtable writing to finish to make sure that when memtables are made immutable, inserts are not going to them. This is currently done in DBImpl::SwitchMemtable(). This is done after flush_scheduler_.TakeNextColumnFamily() is called to fetch the list of column families to switch. The function flush_scheduler_.TakeNextColumnFamily() itself, however, is not thread-safe when being called together with flush_scheduler_.ScheduleFlush(). This change provides a fix, which moves the waiting logic before flush_scheduler_.TakeNextColumnFamily(). WaitForPendingWrites() is a natural place where the logic can happen. Pull Request resolved: facebook#5716 Test Plan: Run all tests with ASAN and TSAN. Differential Revision: D18217658 fbshipit-source-id: b9c5e765c9989645bf10afda7c5c726c3f82f6c3
…ok#5716) Summary: In pipeline writing mode, memtable switching needs to wait for memtable writing to finish to make sure that when memtables are made immutable, inserts are not going to them. This is currently done in DBImpl::SwitchMemtable(). This is done after flush_scheduler_.TakeNextColumnFamily() is called to fetch the list of column families to switch. The function flush_scheduler_.TakeNextColumnFamily() itself, however, is not thread-safe when being called together with flush_scheduler_.ScheduleFlush(). This change provides a fix, which moves the waiting logic before flush_scheduler_.TakeNextColumnFamily(). WaitForPendingWrites() is a natural place where the logic can happen. Pull Request resolved: facebook#5716 Test Plan: Run all tests with ASAN and TSAN. Differential Revision: D18217658 fbshipit-source-id: b9c5e765c9989645bf10afda7c5c726c3f82f6c3
Summary:
In pipeline writing mode, memtable switching needs to wait for memtable writing to finish to make sure that when memtables are made immutable, inserts are not going to them. This is currently done in DBImpl::SwitchMemtable(). This is done after flush_scheduler_.TakeNextColumnFamily() is called to fetch the list of column families to switch. The function flush_scheduler_.TakeNextColumnFamily() itself, however, is not thread-safe when being called together with flush_scheduler_.ScheduleFlush().
This change provides a fix, which moves the waiting logic before flush_scheduler_.TakeNextColumnFamily(). WaitForPendingWrites() is a natural place where the logic can happen.
Test Plan: Run all tests with ASAN and TSAN.