Revamp stress test for LockWAL(), tweak contract #12666
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Upon discovering that LockWAL() does not block file ingestion, I looked into whether its behavior should be expanded to block that or whether it should block as little as reasonably possible to freeze the WAL state. Although the idea of freezing more things that resemble writes was broadly attractive, I found in digging through the code that that would likely expose us to more risks of freezing too much, leading to deadlock. Stopped writes is a good, mature mechanism to reasonably over-approximate what is needed to freeze the WAL state. LockWAL might not even be a useful feature. The MySQL team suspects that FlushWAL would be sufficient for their needs, because writes should already be blocked on their side, but we aren't yet sure we can deprecate the LockWAL feature. So the best course of action now, in this commit and IMHO, is to * Change how we validate LockWAL() in db_stress, checking for changes to GetCurrentWalFile state instead of change to latest sequence number. * Change the contract for LockWAL() to better reflect what we now understand it actually does, which is closer to the original intended functionality. This does not change any LockWAL functionality (and I think we should avoid changing the implementation in the same minor release if we're changing the contract in a minor release). Test Plan: The modifications to how we validate LockWAL() in the stress test mean we can re-enable file ingestion in combination with LockWAL(); HOWEVER, the new validation is bypassed when FaultInjectionTestFS is used because of at least two issues, which are detailed in a FIXME comment in db_stress_test_base.cc where the validation code lives. I've run blackbox_crash_test for quite a while with certain relevant amplifications to ensure it remains stable.
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jowlyzhang
reviewed
May 16, 2024
| // production bug because the old WAL should still be relevant | ||
| // and essentially "current" even if we open a new one. And it's | ||
| // only showing up under fault conditions. | ||
| if (old_wal->LogNumber() != new_wal->LogNumber()) { |
There was a problem hiding this comment.
This is a good solution to make sure wal state does not change. But I suspect there could be such an edge case that doesn't work well with this check. After LockWAL, for the same reason that a file ingestion can go through, a manual flush can possibly go through too. And that can seal the current mutable memtable and switch to a new wal file.
There was a problem hiding this comment.
Good point. Based on some investigation with inserting a mandatory memtable flush in the LockWAL portion of db_stress, it seems like it just blocks, probably because of other parallel writes blocking the flush. Unit test indicates that LockWAL doesn't itself block Flush. New alternate proposal in #12652
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
May 20, 2024
Summary: We recently noticed that some memtable flushed and file ingestions could proceed during LockWAL, in violation of its stated contract. (Note: we aren't 100% sure its actually needed by MySQL, but we want it to be in a clean state nonetheless.) Despite earlier skepticism that this could be done safely (facebook#12666), I found a place to wait to wait for LockWAL to be cleared before allowing these operations to proceed: WaitForPendingWrites() Test Plan: Added to unit tests. Extended how db_stress validates LockWAL and re-enabled combination of ingestion and LockWAL in crash test, in follow-up to facebook#12642 Ran blackbox_crash_test for a long while with relevant features amplified. Suggested follow-up: fix FaultInjectionTestFS to report file sizes consistent with what the user has requested to be flushed.
facebook-github-bot
pushed a commit
that referenced
this pull request
May 21, 2024
Summary: We recently noticed that some memtable flushed and file ingestions could proceed during LockWAL, in violation of its stated contract. (Note: we aren't 100% sure its actually needed by MySQL, but we want it to be in a clean state nonetheless.) Despite earlier skepticism that this could be done safely (#12666), I found a place to wait to wait for LockWAL to be cleared before allowing these operations to proceed: WaitForPendingWrites() Pull Request resolved: #12652 Test Plan: Added to unit tests. Extended how db_stress validates LockWAL and re-enabled combination of ingestion and LockWAL in crash test, in follow-up to #12642 Ran blackbox_crash_test for a long while with relevant features amplified. Suggested follow-up: fix FaultInjectionTestFS to report file sizes consistent with what the user has requested to be flushed. Reviewed By: jowlyzhang Differential Revision: D57622142 Pulled By: pdillinger fbshipit-source-id: aef265fce69465618974b4ec47f4636257c676ce
|
going with #12652 instead |
konstantinvilin
pushed a commit
to konstantinvilin/rocksdb
that referenced
this pull request
May 22, 2024
…12652) Summary: We recently noticed that some memtable flushed and file ingestions could proceed during LockWAL, in violation of its stated contract. (Note: we aren't 100% sure its actually needed by MySQL, but we want it to be in a clean state nonetheless.) Despite earlier skepticism that this could be done safely (facebook#12666), I found a place to wait to wait for LockWAL to be cleared before allowing these operations to proceed: WaitForPendingWrites() Pull Request resolved: facebook#12652 Test Plan: Added to unit tests. Extended how db_stress validates LockWAL and re-enabled combination of ingestion and LockWAL in crash test, in follow-up to facebook#12642 Ran blackbox_crash_test for a long while with relevant features amplified. Suggested follow-up: fix FaultInjectionTestFS to report file sizes consistent with what the user has requested to be flushed. Reviewed By: jowlyzhang Differential Revision: D57622142 Pulled By: pdillinger fbshipit-source-id: aef265fce69465618974b4ec47f4636257c676ce
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Upon discovering that LockWAL() does not block file ingestion, I looked into whether its behavior should be expanded to block that or whether it should block as little as reasonably possible to freeze the WAL state.
Although the idea of freezing more things that resemble writes was broadly attractive, I found in digging through the code that that would likely expose us to more risks of freezing too much, leading to deadlock. Stopped writes is a good, mature mechanism to reasonably over-approximate what is needed to freeze the WAL state.
LockWAL might not even be a useful feature. The MySQL team suspects that FlushWAL would be sufficient for their needs, because writes should already be blocked on their side, but we aren't yet sure we can deprecate the LockWAL feature.
So the best course of action now, in this commit and IMHO, is to
This does not change any LockWAL functionality (and I think we should avoid changing the implementation in the same minor release if we're changing the contract in a minor release).
Follow-up to #12642
Test Plan:
The modifications to how we validate LockWAL() in the stress test mean we can re-enable file ingestion in combination with LockWAL(); HOWEVER, the new validation is bypassed when FaultInjectionTestFS is used because of at least two issues, which are detailed in a FIXME comment in db_stress_test_base.cc where the validation code lives.
I've run blackbox_crash_test for quite a while with certain relevant amplifications to ensure it remains stable.