Skip to content

Commit

Permalink
SPDB-180: general: avoid comparing Status using == as it only compare…
Browse files Browse the repository at this point in the history 
…s status code

This breaks when comparing against e.g. `Status::NoSpace()` because it has
a status code of `Code::kIOError` and only a subcode of `SubCode::kNoSpace`.
  • Loading branch information
@isaac-io @Yuval-Ariel
isaac-io authored and Yuval-Ariel committed Nov 25, 2022
1 parent 62ff2f1 commit 6e10924
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 41 deletions.
2 changes: 1 addition & 1 deletion cache/cache_reservation_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ Status CacheReservationManagerImpl<R>::IncreaseCacheReservation(
return_status = cache_->Insert(GetNextCacheKey(), nullptr, kSizeDummyEntry,
GetNoopDeleterForRole<R>(), &handle);

if (return_status != Status::OK()) {
if (!return_status.ok()) {
return return_status;
}

Expand Down
4 changes: 2 additions & 2 deletions db/db_impl/db_impl_compaction_flush.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ Status DBImpl::FlushMemTableToOutputFile(
error_handler_.SetBGError(s, BackgroundErrorReason::kFlushNoWAL);
}
} else {
assert(s == log_io_s);
assert(s.code() == log_io_s.code() && s.subcode() == log_io_s.subcode());
Status new_bg_error = s;
error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush);
}
Expand Down Expand Up @@ -805,7 +805,7 @@ Status DBImpl::AtomicFlushMemTablesToOutputFiles(
error_handler_.SetBGError(s, BackgroundErrorReason::kFlushNoWAL);
}
} else {
assert(s == log_io_s);
assert(s.code() == log_io_s.code() && s.subcode() == log_io_s.subcode());
Status new_bg_error = s;
error_handler_.SetBGError(new_bg_error, BackgroundErrorReason::kFlush);
}
Expand Down
4 changes: 2 additions & 2 deletions db/error_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,8 +521,8 @@ Status ErrorHandler::OverrideNoSpaceError(const Status& bg_error,

{
uint64_t free_space;
if (db_options_.env->GetFreeSpace(db_options_.db_paths[0].path,
&free_space) == Status::NotSupported()) {
if (db_options_.env->GetFreeSpace(db_options_.db_paths[0].path, &free_space)
.IsNotSupported()) {
*auto_recovery = false;
}
}
Expand Down
39 changes: 26 additions & 13 deletions include/rocksdb/utilities/env_mirror.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,15 @@ class EnvMirror : public EnvWrapper {
std::unique_ptr<Directory> br;
Status as = a_->NewDirectory(name, result);
Status bs = b_->NewDirectory(name, &br);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status FileExists(const std::string& f) override {
Status as = a_->FileExists(f);
Status bs = b_->FileExists(f);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
#if defined(_MSC_VER)
Expand All @@ -79,7 +81,8 @@ class EnvMirror : public EnvWrapper {
std::vector<std::string> ar, br;
Status as = a_->GetChildren(dir, &ar);
Status bs = b_->GetChildren(dir, &br);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
std::sort(ar.begin(), ar.end());
std::sort(br.begin(), br.end());
if (!as.ok() || ar != br) {
Expand All @@ -94,32 +97,37 @@ class EnvMirror : public EnvWrapper {
Status DeleteFile(const std::string& f) override {
Status as = a_->DeleteFile(f);
Status bs = b_->DeleteFile(f);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status CreateDir(const std::string& d) override {
Status as = a_->CreateDir(d);
Status bs = b_->CreateDir(d);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status CreateDirIfMissing(const std::string& d) override {
Status as = a_->CreateDirIfMissing(d);
Status bs = b_->CreateDirIfMissing(d);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status DeleteDir(const std::string& d) override {
Status as = a_->DeleteDir(d);
Status bs = b_->DeleteDir(d);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status GetFileSize(const std::string& f, uint64_t* s) override {
uint64_t asize, bsize;
Status as = a_->GetFileSize(f, &asize);
Status bs = b_->GetFileSize(f, &bsize);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
assert(!as.ok() || asize == bsize);
*s = asize;
return as;
Expand All @@ -130,7 +138,8 @@ class EnvMirror : public EnvWrapper {
uint64_t amtime, bmtime;
Status as = a_->GetFileModificationTime(fname, &amtime);
Status bs = b_->GetFileModificationTime(fname, &bmtime);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
assert(!as.ok() || amtime - bmtime < 10000 || bmtime - amtime < 10000);
*file_mtime = amtime;
return as;
Expand All @@ -139,14 +148,16 @@ class EnvMirror : public EnvWrapper {
Status RenameFile(const std::string& s, const std::string& t) override {
Status as = a_->RenameFile(s, t);
Status bs = b_->RenameFile(s, t);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}

Status LinkFile(const std::string& s, const std::string& t) override {
Status as = a_->LinkFile(s, t);
Status bs = b_->LinkFile(s, t);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}

Expand All @@ -160,7 +171,8 @@ class EnvMirror : public EnvWrapper {
FileLock *al, *bl;
Status as = a_->LockFile(f, &al);
Status bs = b_->LockFile(f, &bl);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
if (as.ok()) *l = new FileLockMirror(al, bl);
return as;
}
Expand All @@ -169,7 +181,8 @@ class EnvMirror : public EnvWrapper {
FileLockMirror* ml = static_cast<FileLockMirror*>(l);
Status as = a_->UnlockFile(ml->a_);
Status bs = b_->UnlockFile(ml->b_);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
delete ml;
return as;
}
Expand Down
2 changes: 1 addition & 1 deletion trace_replay/trace_replay.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ Status TracerHelper::ParseTraceHeader(const Trace& header, int* trace_version,

Status s;
s = ParseVersionStr(t_v_str, trace_version);
if (s != Status::OK()) {
if (!s.ok()) {
return s;
}
s = ParseVersionStr(db_v_str, db_version);
Expand Down
64 changes: 42 additions & 22 deletions utilities/env_mirror.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class SequentialFileMirror : public SequentialFile {
Status Read(size_t n, Slice* result, char* scratch) override {
Slice aslice;
Status as = a_->Read(n, &aslice, scratch);
if (as == Status::OK()) {
if (as.ok()) {
char* bscratch = new char[n];
Slice bslice;
#ifndef NDEBUG
Expand All @@ -34,7 +34,8 @@ class SequentialFileMirror : public SequentialFile {
while (left) {
Status bs = b_->Read(left, &bslice, bscratch);
#ifndef NDEBUG
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
assert(memcmp(bscratch, scratch + off, bslice.size()) == 0);
off += bslice.size();
#endif
Expand All @@ -44,21 +45,24 @@ class SequentialFileMirror : public SequentialFile {
*result = aslice;
} else {
Status bs = b_->Read(n, result, scratch);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
}
return as;
}

Status Skip(uint64_t n) override {
Status as = a_->Skip(n);
Status bs = b_->Skip(n);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status InvalidateCache(size_t offset, size_t length) override {
Status as = a_->InvalidateCache(offset, length);
Status bs = b_->InvalidateCache(offset, length);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
};
};
Expand All @@ -72,22 +76,24 @@ class RandomAccessFileMirror : public RandomAccessFile {
Status Read(uint64_t offset, size_t n, Slice* result,
char* scratch) const override {
Status as = a_->Read(offset, n, result, scratch);
if (as == Status::OK()) {
if (as.ok()) {
char* bscratch = new char[n];
Slice bslice;
size_t off = 0;
size_t left = result->size();
while (left) {
Status bs = b_->Read(offset + off, left, &bslice, bscratch);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
assert(memcmp(bscratch, scratch + off, bslice.size()) == 0);
off += bslice.size();
left -= bslice.size();
}
delete[] bscratch;
} else {
Status bs = b_->Read(offset, n, result, scratch);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
}
return as;
}
Expand All @@ -108,7 +114,8 @@ class WritableFileMirror : public WritableFile {
Status Append(const Slice& data) override {
Status as = a_->Append(data);
Status bs = b_->Append(data);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status Append(const Slice& data,
Expand All @@ -118,7 +125,8 @@ class WritableFileMirror : public WritableFile {
Status PositionedAppend(const Slice& data, uint64_t offset) override {
Status as = a_->PositionedAppend(data, offset);
Status bs = b_->PositionedAppend(data, offset);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status PositionedAppend(
Expand All @@ -129,31 +137,36 @@ class WritableFileMirror : public WritableFile {
Status Truncate(uint64_t size) override {
Status as = a_->Truncate(size);
Status bs = b_->Truncate(size);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status Close() override {
Status as = a_->Close();
Status bs = b_->Close();
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status Flush() override {
Status as = a_->Flush();
Status bs = b_->Flush();
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status Sync() override {
Status as = a_->Sync();
Status bs = b_->Sync();
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status Fsync() override {
Status as = a_->Fsync();
Status bs = b_->Fsync();
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
bool IsSyncThreadSafe() const override {
Expand Down Expand Up @@ -186,21 +199,24 @@ class WritableFileMirror : public WritableFile {
Status InvalidateCache(size_t offset, size_t length) override {
Status as = a_->InvalidateCache(offset, length);
Status bs = b_->InvalidateCache(offset, length);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}

protected:
Status Allocate(uint64_t offset, uint64_t length) override {
Status as = a_->Allocate(offset, length);
Status bs = b_->Allocate(offset, length);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
Status RangeSync(uint64_t offset, uint64_t nbytes) override {
Status as = a_->RangeSync(offset, nbytes);
Status bs = b_->RangeSync(offset, nbytes);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
return as;
}
};
Expand All @@ -214,7 +230,8 @@ Status EnvMirror::NewSequentialFile(const std::string& f,
SequentialFileMirror* mf = new SequentialFileMirror(f);
Status as = a_->NewSequentialFile(f, &mf->a_, options);
Status bs = b_->NewSequentialFile(f, &mf->b_, options);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
if (as.ok())
r->reset(mf);
else
Expand All @@ -231,7 +248,8 @@ Status EnvMirror::NewRandomAccessFile(const std::string& f,
RandomAccessFileMirror* mf = new RandomAccessFileMirror(f);
Status as = a_->NewRandomAccessFile(f, &mf->a_, options);
Status bs = b_->NewRandomAccessFile(f, &mf->b_, options);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
if (as.ok())
r->reset(mf);
else
Expand All @@ -246,7 +264,8 @@ Status EnvMirror::NewWritableFile(const std::string& f,
WritableFileMirror* mf = new WritableFileMirror(f, options);
Status as = a_->NewWritableFile(f, &mf->a_, options);
Status bs = b_->NewWritableFile(f, &mf->b_, options);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
if (as.ok())
r->reset(mf);
else
Expand All @@ -263,7 +282,8 @@ Status EnvMirror::ReuseWritableFile(const std::string& fname,
WritableFileMirror* mf = new WritableFileMirror(fname, options);
Status as = a_->ReuseWritableFile(fname, old_fname, &mf->a_, options);
Status bs = b_->ReuseWritableFile(fname, old_fname, &mf->b_, options);
assert(as == bs);
assert(as.code() == bs.code());
assert(as.subcode() == bs.subcode());
if (as.ok())
r->reset(mf);
else
Expand Down

0 comments on commit 6e10924

Please sign in to comment.