WritableFileWriter to allow operation after failure when SyncWithoutFlush() is involved #10555
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
…lush() is involved Summary: facebook#10489 adds an assertion in most functions in WritableFileWriter to check no previous error. However, it only works without calling SyncWithoutFlush(). The nature of SyncWithoutFlush() makes two concurrent call fails to check status code of each other and causing assertion failure. Fix the problem by skipping the check after SyncWithoutFlush() is called and not check status code in SyncWithoutFlush(). Since the original change was not officially released yet, the fix isn't added to HISTORY.md. Test Plan: Make sure existing tests still pass
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
anand1976
approved these changes
Aug 23, 2022
gitbw95
pushed a commit
to gitbw95/rocksdb
that referenced
this pull request
Aug 24, 2022
…lush() is involved (facebook#10555) Summary: facebook#10489 adds an assertion in most functions in WritableFileWriter to check no previous error. However, it only works without calling SyncWithoutFlush(). The nature of SyncWithoutFlush() makes two concurrent call fails to check status code of each other and causing assertion failure. Fix the problem by skipping the check after SyncWithoutFlush() is called and not check status code in SyncWithoutFlush(). Since the original change was not officially released yet, the fix isn't added to HISTORY.md. Pull Request resolved: facebook#10555 Test Plan: Make sure existing tests still pass Reviewed By: anand1976 Differential Revision: D38946208 fbshipit-source-id: 63566732d3f25c8a8342840499cf7b7d745f27c2
Merged
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 24, 2023
…11996) Summary: **Context/Summary:** We ignore trace writing status e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222 If a write into the trace file fails, subsequent trace write will continue onto the same file. This will trigger the assertion `assert(sync_without_flush_called_)` intended to catch write to a file that has previously seen error, added in #10489, #10555 Alternative (rejected) is to handle trace writing status at a higher level at e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222. However, it makes sense to ignore such status considering tracing is not a critical but assistant component to db operation. And this alternative requires more code change. So it's better to handle the failure at a lower level as this PR Pull Request resolved: #11996 Test Plan: Add new UT failed before this PR and pass after Reviewed By: akankshamahajan15 Differential Revision: D50532467 Pulled By: hx235 fbshipit-source-id: f2032abafd94917adbf89a20841d15b448782a33
rockeet
pushed a commit
to topling/toplingdb
that referenced
this pull request
Dec 23, 2023
…#11996) Summary: **Context/Summary:** We ignore trace writing status e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222 If a write into the trace file fails, subsequent trace write will continue onto the same file. This will trigger the assertion `assert(sync_without_flush_called_)` intended to catch write to a file that has previously seen error, added in facebook/rocksdb#10489, facebook/rocksdb#10555 Alternative (rejected) is to handle trace writing status at a higher level at e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222. However, it makes sense to ignore such status considering tracing is not a critical but assistant component to db operation. And this alternative requires more code change. So it's better to handle the failure at a lower level as this PR Pull Request resolved: facebook/rocksdb#11996 Test Plan: Add new UT failed before this PR and pass after Reviewed By: akankshamahajan15 Differential Revision: D50532467 Pulled By: hx235 fbshipit-source-id: f2032abafd94917adbf89a20841d15b448782a33
rockeet
pushed a commit
to topling/toplingdb
that referenced
this pull request
Sep 1, 2024
…#11996) Summary: **Context/Summary:** We ignore trace writing status e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222 If a write into the trace file fails, subsequent trace write will continue onto the same file. This will trigger the assertion `assert(sync_without_flush_called_)` intended to catch write to a file that has previously seen error, added in facebook/rocksdb#10489, facebook/rocksdb#10555 Alternative (rejected) is to handle trace writing status at a higher level at e.g, https://github.com/facebook/rocksdb/blob/543191f2eacadf14e3aa6ff9a08f85a8ad82da95/db/db_impl/db_impl_write.cc#L221-L222. However, it makes sense to ignore such status considering tracing is not a critical but assistant component to db operation. And this alternative requires more code change. So it's better to handle the failure at a lower level as this PR Pull Request resolved: facebook/rocksdb#11996 Test Plan: Add new UT failed before this PR and pass after Reviewed By: akankshamahajan15 Differential Revision: D50532467 Pulled By: hx235 fbshipit-source-id: f2032abafd94917adbf89a20841d15b448782a33
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:
#10489 adds an assertion in most functions in WritableFileWriter to check no previous error. However, it only works without calling SyncWithoutFlush(). The nature of SyncWithoutFlush() makes two concurrent call fails to check status code of each other and causing assertion failure. Fix the problem by skipping the check after SyncWithoutFlush() is called and not check status code in SyncWithoutFlush().
Since the original change was not officially released yet, the fix isn't added to HISTORY.md.
Test Plan: Make sure existing tests still pass