From 77a28615eceb9dcbb61c9090ce6a799da358a7fd Mon Sep 17 00:00:00 2001 From: agiardullo Date: Fri, 14 Aug 2015 16:45:05 -0700 Subject: [PATCH] Support static Status messages Summary: Provide a way to specify a detailed static error message for a Status without incurring a memcpy. Let me know what people think of this approach. Test Plan: added simple test Reviewers: igor, yhchiang, rven, sdong Reviewed By: sdong Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D44259 --- CMakeLists.txt | 1 + include/rocksdb/status.h | 113 ++++++++++++------ src.mk | 1 + util/status.cc | 9 +- util/status_message.cc | 17 +++ .../transactions/transaction_lock_mgr.cc | 7 +- utilities/transactions/transaction_test.cc | 1 + 7 files changed, 107 insertions(+), 42 deletions(-) create mode 100644 util/status_message.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 66dbd83608..1f5207366d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -191,6 +191,7 @@ set(SOURCES util/sst_dump_tool.cc util/statistics.cc util/status.cc + util/status_message.cc util/string_util.cc util/sync_point.cc util/testharness.cc diff --git a/include/rocksdb/status.h b/include/rocksdb/status.h index 043ca139ff..e8e7970ccf 100644 --- a/include/rocksdb/status.h +++ b/include/rocksdb/status.h @@ -25,7 +25,7 @@ namespace rocksdb { class Status { public: // Create a success status. - Status() : code_(kOk), state_(nullptr) { } + Status() : code_(kOk), subcode_(kNone), state_(nullptr) {} ~Status() { delete[] state_; } // Copy the specified status. @@ -34,6 +34,35 @@ class Status { bool operator==(const Status& rhs) const; bool operator!=(const Status& rhs) const; + enum Code { + kOk = 0, + kNotFound = 1, + kCorruption = 2, + kNotSupported = 3, + kInvalidArgument = 4, + kIOError = 5, + kMergeInProgress = 6, + kIncomplete = 7, + kShutdownInProgress = 8, + kTimedOut = 9, + kAborted = 10, + kBusy = 11, + kExpired = 12, + kTryAgain = 13 + }; + + Code code() const { return code_; } + + enum SubCode { + kNone = 0, + kMutexTimeout = 1, + kLockTimeout = 2, + kLockLimit = 3, + kMaxSubCode + }; + + SubCode subcode() const { return subcode_; } + // Return a success status. static Status OK() { return Status(); } @@ -42,51 +71,76 @@ class Status { return Status(kNotFound, msg, msg2); } // Fast path for not found without malloc; - static Status NotFound() { - return Status(kNotFound); - } + static Status NotFound(SubCode msg = kNone) { return Status(kNotFound, msg); } + static Status Corruption(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kCorruption, msg, msg2); } + static Status Corruption(SubCode msg = kNone) { + return Status(kCorruption, msg); + } + static Status NotSupported(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kNotSupported, msg, msg2); } + static Status NotSupported(SubCode msg = kNone) { + return Status(kNotSupported, msg); + } + static Status InvalidArgument(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kInvalidArgument, msg, msg2); } + static Status InvalidArgument(SubCode msg = kNone) { + return Status(kInvalidArgument, msg); + } + static Status IOError(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kIOError, msg, msg2); } - static Status MergeInProgress() { return Status(kMergeInProgress); } + static Status IOError(SubCode msg = kNone) { return Status(kIOError, msg); } + + static Status MergeInProgress(const Slice& msg, const Slice& msg2 = Slice()) { + return Status(kMergeInProgress, msg, msg2); + } + static Status MergeInProgress(SubCode msg = kNone) { + return Status(kMergeInProgress, msg); + } + static Status Incomplete(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kIncomplete, msg, msg2); } - static Status ShutdownInProgress() { - return Status(kShutdownInProgress); + static Status Incomplete(SubCode msg = kNone) { + return Status(kIncomplete, msg); + } + + static Status ShutdownInProgress(SubCode msg = kNone) { + return Status(kShutdownInProgress, msg); } static Status ShutdownInProgress(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kShutdownInProgress, msg, msg2); } - static Status Aborted() { - return Status(kAborted); - } + static Status Aborted(SubCode msg = kNone) { return Status(kAborted, msg); } static Status Aborted(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kAborted, msg, msg2); } - static Status Busy() { return Status(kBusy); } + + static Status Busy(SubCode msg = kNone) { return Status(kBusy, msg); } static Status Busy(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kBusy, msg, msg2); } - static Status TimedOut() { return Status(kTimedOut); } + + static Status TimedOut(SubCode msg = kNone) { return Status(kTimedOut, msg); } static Status TimedOut(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kTimedOut, msg, msg2); } - static Status Expired() { return Status(kExpired); } + + static Status Expired(SubCode msg = kNone) { return Status(kExpired, msg); } static Status Expired(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kExpired, msg, msg2); } - static Status TryAgain() { return Status(kTryAgain); } + + static Status TryAgain(SubCode msg = kNone) { return Status(kTryAgain, msg); } static Status TryAgain(const Slice& msg, const Slice& msg2 = Slice()) { return Status(kTryAgain, msg, msg2); } @@ -138,26 +192,6 @@ class Status { // Returns the string "OK" for success. std::string ToString() const; - enum Code { - kOk = 0, - kNotFound = 1, - kCorruption = 2, - kNotSupported = 3, - kInvalidArgument = 4, - kIOError = 5, - kMergeInProgress = 6, - kIncomplete = 7, - kShutdownInProgress = 8, - kTimedOut = 9, - kAborted = 10, - kBusy = 11, - kExpired = 12, - kTryAgain = 13 - }; - - Code code() const { - return code_; - } private: // A nullptr state_ (which is always the case for OK) means the message // is empty. @@ -165,21 +199,26 @@ class Status { // state_[0..3] == length of message // state_[4..] == message Code code_; + SubCode subcode_; const char* state_; - explicit Status(Code _code) : code_(_code), state_(nullptr) {} + static const char* msgs[static_cast(kMaxSubCode)]; + + explicit Status(Code _code, SubCode _subcode = kNone) + : code_(_code), subcode_(_subcode), state_(nullptr) {} + Status(Code _code, const Slice& msg, const Slice& msg2); static const char* CopyState(const char* s); }; -inline Status::Status(const Status& s) { - code_ = s.code_; +inline Status::Status(const Status& s) : code_(s.code_), subcode_(s.subcode_) { state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); } inline void Status::operator=(const Status& s) { // The following condition catches both aliasing (when this == &s), // and the common case where both s and *this are ok. code_ = s.code_; + subcode_ = s.subcode_; if (state_ != s.state_) { delete[] state_; state_ = (s.state_ == nullptr) ? nullptr : CopyState(s.state_); diff --git a/src.mk b/src.mk index 6bb99f287d..96b3cadb1b 100644 --- a/src.mk +++ b/src.mk @@ -145,6 +145,7 @@ LIB_SOURCES = \ util/sst_dump_tool.cc \ util/statistics.cc \ util/status.cc \ + util/status_message.cc \ util/string_util.cc \ util/sync_point.cc \ util/thread_local.cc \ diff --git a/util/status.cc b/util/status.cc index 3935dc561f..6ff5005f9e 100644 --- a/util/status.cc +++ b/util/status.cc @@ -21,7 +21,8 @@ const char* Status::CopyState(const char* state) { return result; } -Status::Status(Code _code, const Slice& msg, const Slice& msg2) : code_(_code) { +Status::Status(Code _code, const Slice& msg, const Slice& msg2) + : code_(_code), subcode_(kNone) { assert(code_ != kOk); const uint32_t len1 = static_cast(msg.size()); const uint32_t len2 = static_cast(msg2.size()); @@ -89,6 +90,12 @@ std::string Status::ToString() const { break; } std::string result(type); + if (subcode_ != kNone) { + uint32_t index = static_cast(subcode_); + assert(sizeof(msgs) > index); + result.append(msgs[index]); + } + if (state_ != nullptr) { uint32_t length; memcpy(&length, state_, sizeof(length)); diff --git a/util/status_message.cc b/util/status_message.cc new file mode 100644 index 0000000000..26ab06ddde --- /dev/null +++ b/util/status_message.cc @@ -0,0 +1,17 @@ +// Copyright (c) 2015, Facebook, Inc. All rights reserved. +// This source code is licensed under the BSD-style license found in the +// LICENSE file in the root directory of this source tree. An additional grant +// of patent rights can be found in the PATENTS file in the same directory. + +#include "rocksdb/status.h" + +namespace rocksdb { + +const char* Status::msgs[] = { + "", // kNone + "Timeout Acquiring Mutex", // kMutexTimeout + "Timeout waiting to lock key", // kLockTimeout + "Failed to acquire lock due to max_num_locks limit" // kLockLimit +}; + +} // namespace rocksdb diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc index c7a327202a..882c441d91 100644 --- a/utilities/transactions/transaction_lock_mgr.cc +++ b/utilities/transactions/transaction_lock_mgr.cc @@ -232,7 +232,7 @@ Status TransactionLockMgr::AcquireWithTimeout(LockMap* lock_map, if (!locked) { // timeout acquiring mutex - return Status::TimedOut("Timeout Acquiring Mutex"); + return Status::TimedOut(Status::SubCode::kMutexTimeout); } // Acquire lock if we are able to @@ -313,15 +313,14 @@ Status TransactionLockMgr::AcquireLocked(LockMap* lock_map, lock_info.expiration_time = txn_lock_info.expiration_time; // lock_cnt does not change } else { - result = Status::TimedOut("lock held"); + result = Status::TimedOut(Status::SubCode::kLockTimeout); } } } else { // Lock not held. // Check lock limit if (max_num_locks_ > 0 && lock_map->lock_cnt.load(std::memory_order_acquire) >= max_num_locks_) { - result = - Status::Busy("Failed to acquire lock due to max_num_locks limit"); + result = Status::Busy(Status::SubCode::kLockLimit); } else { // acquire lock stripe->keys.insert({key, txn_lock_info}); diff --git a/utilities/transactions/transaction_test.cc b/utilities/transactions/transaction_test.cc index 0be31716e8..e9d6a196f4 100644 --- a/utilities/transactions/transaction_test.cc +++ b/utilities/transactions/transaction_test.cc @@ -1564,6 +1564,7 @@ TEST_F(TransactionTest, TimeoutTest) { // txn2 has a smaller lock timeout than txn1's expiration, so it will time out s = txn2->Delete("asdf"); ASSERT_TRUE(s.IsTimedOut()); + ASSERT_EQ(s.ToString(), "Operation timed out: Timeout waiting to lock key"); s = txn1->Commit(); ASSERT_OK(s);