Add more tests to ASSERT_STATUS_CHECKED (4) #7718
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
mrambacher
reviewed
Dec 2, 2020
db/db_impl/db_impl_write.cc
Outdated
Comment on lines
577
to
582
|
|
||
| // The status will never be written or read and this | ||
| // is needed to avoid ASSERT_STATUS_CHECKED failure | ||
| // when the memtable_write_group is free'd | ||
| memtable_write_group.status.PermitUncheckedError(); | ||
|
|
There was a problem hiding this comment.
Should we just do this in the constructor since this is effectively an unused status?
There was a problem hiding this comment.
I felt that this was likely specific to just how it is used here... What do you think?
adamretter
force-pushed
the
assert-status-checked-4
branch
2 times, most recently
from
6dc26f1
to
8751e52
Compare
adamretter
force-pushed
the
assert-status-checked-4
branch
from
2f50ac0
to
cb46dc3
Compare
adamretter
force-pushed
the
assert-status-checked-4
branch
from
cb46dc3
to
9ff22cb
Compare
siying
approved these changes
Dec 21, 2020
| @@ -1307,6 +1321,7 @@ TEST_F(DBRangeDelTest, UntruncatedTombstoneDoesNotDeleteNewerKey) { | |||
|
|
|||
| auto get_key_count = [this]() -> int { | |||
| auto* iter = db_->NewIterator(ReadOptions()); | |||
| assert(iter->status().ok()); | |||
There was a problem hiding this comment.
why not ASSERT_OK()? If it doesn't work how about EXPECT_OK()?
| if (!TryMerge(&aligned_reqs.back(), r)) { | ||
| if (i == 0) { | ||
| // head | ||
| aligned_reqs.push_back(r); |
There was a problem hiding this comment.
Moving this into the loop doesn't seem to be relevant to this PR. Whether it makes it easier to read is arguable and the performance won't change much but doesn't seem to improve much either. So I would suggest we separate this change.
There was a problem hiding this comment.
Seems to be for narrowing down the case requiring r.status.PermitUncheckedError();
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in 81592d9. |
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 6, 2021
…7838) Summary: The IOStatus of TableBuilder is returned by copy the io status from builder->io_status(). pr #7718 swallowed the io status and it will cause the write IO error become non-retryable and no auto resume logic will handle it. Roll back to previous implementation. Pull Request resolved: #7838 Test Plan: make check Reviewed By: ajkr Differential Revision: D25795387 Pulled By: zhichao-cao fbshipit-source-id: bc35e69e0b71aa4148a6ed76f073357041b8e372
codingrhythm
pushed a commit
to SafetyCulture/rocksdb
that referenced
this pull request
Mar 5, 2021
Summary: Fourth batch of adding more tests to ASSERT_STATUS_CHECKED. * db_range_del_test * db_write_test * random_access_file_reader_test * merge_test * external_sst_file_test * write_buffer_manager_test * stringappend_test * deletefile_test Pull Request resolved: facebook#7718 Reviewed By: pdillinger Differential Revision: D25671608 fbshipit-source-id: 687a794e98a9e0cd5428ead9898ef05ced987c31
codingrhythm
pushed a commit
to SafetyCulture/rocksdb
that referenced
this pull request
Mar 5, 2021
…acebook#7838) Summary: The IOStatus of TableBuilder is returned by copy the io status from builder->io_status(). pr facebook#7718 swallowed the io status and it will cause the write IO error become non-retryable and no auto resume logic will handle it. Roll back to previous implementation. Pull Request resolved: facebook#7838 Test Plan: make check Reviewed By: ajkr Differential Revision: D25795387 Pulled By: zhichao-cao fbshipit-source-id: bc35e69e0b71aa4148a6ed76f073357041b8e372
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 18, 2021
Summary: Fourth batch of adding more tests to ASSERT_STATUS_CHECKED. * db_range_del_test * db_write_test * random_access_file_reader_test * merge_test * external_sst_file_test * write_buffer_manager_test * stringappend_test * deletefile_test Pull Request resolved: facebook/rocksdb#7718 Reviewed By: pdillinger Differential Revision: D25671608 fbshipit-source-id: 687a794e98a9e0cd5428ead9898ef05ced987c31 Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 23, 2021
Summary: Fourth batch of adding more tests to ASSERT_STATUS_CHECKED. * db_range_del_test * db_write_test * random_access_file_reader_test * merge_test * external_sst_file_test * write_buffer_manager_test * stringappend_test * deletefile_test Pull Request resolved: facebook/rocksdb#7718 Reviewed By: pdillinger Differential Revision: D25671608 fbshipit-source-id: 687a794e98a9e0cd5428ead9898ef05ced987c31 Signed-off-by: Changlong Chen <levisonchen@live.cn>
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.
Fourth batch of adding more tests to ASSERT_STATUS_CHECKED.