Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing status check when compiling with ASSERT_STATUS_CHECKED=1 #11686

Closed
wants to merge 2 commits into from
Closed

Add missing status check when compiling with ASSERT_STATUS_CHECKED=1 #11686

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfb95c5
I thought it's an easy fix.
cbi42 Aug 8, 2023
9820945
Address comment
cbi42 Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Address comment
  • Loading branch information
@cbi42
cbi42 committed Aug 9, 2023
commit 982094575737716b455a685ca866faded2a58099
4 changes: 2 additions & 2 deletions options/options_settable_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ namespace ROCKSDB_NAMESPACE {
#if defined OS_LINUX || defined OS_WIN
#ifndef __clang__
#ifndef ROCKSDB_UBSAN_RUN
// status check adds CXX flag -fno-elide-constructors which fails this test.
#ifndef ROCKSDB_ASSERT_STATUS_CHECKED

class OptionsSettableTest : public testing::Test {
public:
Expand Down Expand Up @@ -380,6 +378,8 @@ TEST_F(OptionsSettableTest, DBOptionsAllFieldsSettable) {
delete[] new_options_ptr;
}

// status check adds CXX flag -fno-elide-constructors which fails this test.
#ifndef ROCKSDB_ASSERT_STATUS_CHECKED
// If the test fails, likely a new option is added to ColumnFamilyOptions
// but it cannot be set through GetColumnFamilyOptionsFromString(), or the
// test is not updated accordingly.
Expand Down
6 changes: 6 additions & 0 deletions utilities/blob_db/blob_db_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,12 @@ Status BlobDBImpl::Open(std::vector<ColumnFamilyHandle*>* handles) {
SstFileManagerImpl* sfm = static_cast<SstFileManagerImpl*>(
db_impl_->immutable_db_options().sst_file_manager.get());
s = DeleteScheduler::CleanupDirectory(env_, sfm, blob_dir_);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change in non-unit-test code.

Copy link
Contributor

@hx235 hx235 Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest following the pattern in this function that if(!s.ok()) {return s;} (optionally with some error logging if you'd like) following a s=... as we can't guarantee s = DeleteScheduler::... is the last s of the function in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added status check and error logging.

if (!s.ok()) {
ROCKS_LOG_ERROR(db_options_.info_log,
"Failed to clean up directory %s, status: %s",
blob_dir_.c_str(), s.ToString().c_str());
return s;
}

UpdateLiveSSTSize();

Expand Down