From d35496e4b71233c0c0d01c590b848f110211bc5c Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Sat, 4 Apr 2020 00:12:06 +0800 Subject: [PATCH 1/3] Check the create edge index request early. --- src/common/base/Status.h | 100 +++++++++--------- src/graph/CreateEdgeIndexExecutor.cpp | 29 +++-- .../indexMan/CreateEdgeIndexProcessor.cpp | 27 ++--- 3 files changed, 83 insertions(+), 73 deletions(-) diff --git a/src/common/base/Status.h b/src/common/base/Status.h index c2bb7d0dc7d..d2cc10d02a5 100644 --- a/src/common/base/Status.h +++ b/src/common/base/Status.h @@ -29,7 +29,7 @@ class Status final { state_ = rhs.state_ == nullptr ? nullptr : copyState(rhs.state_.get()); } - Status& operator=(const Status &rhs) { + Status &operator=(const Status &rhs) { // `state_ == rhs.state_' means either `this == &rhs', // or both `*this' and `rhs' are OK if (state_ != rhs.state_) { @@ -42,7 +42,7 @@ class Status final { state_ = std::move(rhs.state_); } - Status& operator=(Status &&rhs) noexcept { + Status &operator=(Status &&rhs) noexcept { // `state_ == rhs.state_' means either `this == &rhs', // or both `*this' and `rhs' are OK if (state_ != rhs.state_) { @@ -72,26 +72,25 @@ class Status final { return Status(); } -#define STATUS_GENERATOR(ERROR) \ - static Status ERROR() { \ - return Status(k##ERROR, ""); \ - } \ - \ - static Status ERROR(folly::StringPiece msg) { \ - return Status(k##ERROR, msg); \ - } \ - \ - static Status ERROR(const char *fmt, ...) \ - __attribute__((format(printf, 1, 2))) { \ - va_list args; \ - va_start(args, fmt); \ - auto msg = format(fmt, args); \ - va_end(args); \ - return Status(k##ERROR, msg); \ - } \ - \ - bool is##ERROR() const { \ - return code() == k##ERROR; \ +#define STATUS_GENERATOR(ERROR) \ + static Status ERROR() { \ + return Status(k##ERROR, ""); \ + } \ + \ + static Status ERROR(folly::StringPiece msg) { \ + return Status(k##ERROR, msg); \ + } \ + \ + static Status ERROR(const char *fmt, ...) __attribute__((format(printf, 1, 2))) { \ + va_list args; \ + va_start(args, fmt); \ + auto msg = format(fmt, args); \ + va_end(args); \ + return Status(k##ERROR, msg); \ + } \ + \ + bool is##ERROR() const { \ + return code() == k##ERROR; \ } // Some succeeded codes STATUS_GENERATOR(Inserted); @@ -103,6 +102,7 @@ class Status final { // Graph engine errors STATUS_GENERATOR(SyntaxError); + STATUS_GENERATOR(MalformedRequest); // Nothing is executed When command is comment STATUS_GENERATOR(StatementEmpty); @@ -128,49 +128,52 @@ class Status final { std::string toString() const; - friend std::ostream& operator<<(std::ostream &os, const Status &status); + friend std::ostream &operator<<(std::ostream &os, const Status &status); // If some kind of error really needs to be distinguished with others using a specific // code, other than a general code and specific msg, you could add a new code below, // e.g. kSomeError, and add the cooresponding STATUS_GENERATOR(SomeError) enum Code : uint16_t { + // clang-format off // OK - kOk = 0, - kInserted = 1, + kOk = 0, + kInserted = 1, // 1xx, for general errors - kError = 101, - kNoSuchFile = 102, - kNotSupported = 103, + kError = 101, + kNoSuchFile = 102, + kNotSupported = 103, // 2xx, for graph engine errors - kSyntaxError = 201, - kStatementEmpty = 202, + kSyntaxError = 201, + kStatementEmpty = 202, + kMalformedRequest = 203, // 3xx, for storage engine errors - kKeyNotFound = 301, + kKeyNotFound = 301, // 4xx, for meta service errors - kSpaceNotFound = 404, - kHostNotFound = 405, - kTagNotFound = 406, - kEdgeNotFound = 407, - kUserNotFound = 408, - kLeaderChanged = 409, - kBalanced = 410, - kIndexNotFound = 411, - kPartNotFound = 412, + kSpaceNotFound = 404, + kHostNotFound = 405, + kTagNotFound = 406, + kEdgeNotFound = 407, + kUserNotFound = 408, + kLeaderChanged = 409, + kBalanced = 410, + kIndexNotFound = 411, + kPartNotFound = 412, // 5xx for user or permission error - kPermissionError = 501, + kPermissionError = 501, + // clang-format on }; Code code() const { if (state_ == nullptr) { return kOk; } - return reinterpret_cast(state_.get())->code_; + return reinterpret_cast(state_.get())->code_; } private: // REQUIRES: stat_ != nullptr uint16_t size() const { - return reinterpret_cast(state_.get())->size_; + return reinterpret_cast(state_.get())->size_; } Status(Code code, folly::StringPiece msg); @@ -181,8 +184,8 @@ class Status final { private: struct Header { - uint16_t size_; - Code code_; + uint16_t size_; + Code code_; }; static constexpr auto kHeaderSize = sizeof(Header); // state_ == nullptr indicates OK @@ -190,14 +193,13 @@ class Status final { // state_[0..1] length of the error msg, i.e. size() - kHeaderSize // state_[2..3] code // state_[4...] verbose error message - std::unique_ptr state_; + std::unique_ptr state_; }; - -inline std::ostream& operator<<(std::ostream &os, const Status &status) { +inline std::ostream &operator<<(std::ostream &os, const Status &status) { return os << status.toString(); } } // namespace nebula -#endif // COMMON_BASE_STATUS_H_ +#endif // COMMON_BASE_STATUS_H_ diff --git a/src/graph/CreateEdgeIndexExecutor.cpp b/src/graph/CreateEdgeIndexExecutor.cpp index fec9a46e49a..796554cf264 100644 --- a/src/graph/CreateEdgeIndexExecutor.cpp +++ b/src/graph/CreateEdgeIndexExecutor.cpp @@ -9,9 +9,9 @@ namespace nebula { namespace graph { -CreateEdgeIndexExecutor::CreateEdgeIndexExecutor(Sentence *sentence, - ExecutionContext *ectx) : Executor(ectx) { - sentence_ = static_cast(sentence); +CreateEdgeIndexExecutor::CreateEdgeIndexExecutor(Sentence *sentence, ExecutionContext *ectx) + : Executor(ectx) { + sentence_ = static_cast(sentence); } Status CreateEdgeIndexExecutor::prepare() { @@ -31,14 +31,22 @@ void CreateEdgeIndexExecutor::execute() { auto *edgeName = sentence_->edgeName(); auto columns = sentence_->names(); auto spaceId = ectx()->rctx()->session()->space(); + if (columns.empty()) { + onError_(Status::MalformedRequest("Empty fields.")); + return; + } + std::unordered_set uniFields; + uniFields.reserve(columns.size()); + uniFields.insert(columns.begin(), columns.end()); + if (uniFields.size() != columns.size()) { + onError_(Status::MalformedRequest("Duplicate fields.")); + return; + } - auto future = mc->createEdgeIndex(spaceId, - *name, - *edgeName, - columns, - sentence_->isIfNotExist()); + auto future = + mc->createEdgeIndex(spaceId, *name, *edgeName, columns, sentence_->isIfNotExist()); auto *runner = ectx()->rctx()->runner(); - auto cb = [this] (auto &&resp) { + auto cb = [this](auto &&resp) { if (!resp.ok()) { DCHECK(onError_); onError_(resp.status()); @@ -49,7 +57,7 @@ void CreateEdgeIndexExecutor::execute() { onFinish_(Executor::ProcessControl::kNext); }; - auto error = [this] (auto &&e) { + auto error = [this](auto &&e) { LOG(ERROR) << "Exception caught: " << e.what(); onError_(Status::Error("Internal error")); }; @@ -59,4 +67,3 @@ void CreateEdgeIndexExecutor::execute() { } // namespace graph } // namespace nebula - diff --git a/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp b/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp index 334a4e0f7aa..335a889ffc0 100644 --- a/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp +++ b/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp @@ -9,9 +9,7 @@ namespace nebula { namespace meta { -void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) { - auto space = req.get_space_id(); - CHECK_SPACE_ID_AND_RETURN(space); +void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq &req) { const auto &indexName = req.get_index_name(); auto &edgeName = req.get_edge_name(); auto &fieldNames = req.get_fields(); @@ -30,6 +28,9 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) { return; } + auto space = req.get_space_id(); + CHECK_SPACE_ID_AND_RETURN(space); + folly::SharedMutex::WriteHolder wHolder(LockUtils::edgeIndexLock()); auto ret = getIndexID(space, indexName); if (ret.ok()) { @@ -62,17 +63,18 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) { auto latestEdgeSchema = retSchema.value(); if (tagOrEdgeHasTTL(latestEdgeSchema)) { - LOG(ERROR) << "Edge: " << edgeName << " has ttl, not create index"; - handleErrorCode(cpp2::ErrorCode::E_INDEX_WITH_TTL); - onFinished(); - return; + LOG(ERROR) << "Edge: " << edgeName << " has ttl, not create index"; + handleErrorCode(cpp2::ErrorCode::E_INDEX_WITH_TTL); + onFinished(); + return; } auto fields = getLatestEdgeFields(latestEdgeSchema); std::vector columns; for (auto &field : fieldNames) { - auto iter = std::find_if(std::begin(fields), std::end(fields), - [field](const auto& pair) { return field == pair.first; }); + auto iter = std::find_if(std::begin(fields), std::end(fields), [field](const auto &pair) { + return field == pair.first; + }); if (iter == fields.end()) { LOG(ERROR) << "Field " << field << " not found in Edge " << edgeName; @@ -108,7 +110,7 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) { item.set_fields(std::move(columns)); data.emplace_back(MetaServiceUtils::indexIndexKey(space, indexName), - std::string(reinterpret_cast(&edgeIndex), sizeof(IndexID))); + std::string(reinterpret_cast(&edgeIndex), sizeof(IndexID))); data.emplace_back(MetaServiceUtils::indexKey(space, edgeIndex), MetaServiceUtils::indexVal(item)); LOG(INFO) << "Create Edge Index " << indexName << ", edgeIndex " << edgeIndex; @@ -116,6 +118,5 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq& req) { doSyncPutAndUpdate(std::move(data)); } -} // namespace meta -} // namespace nebula - +} // namespace meta +} // namespace nebula From 4c3747beb910ba444adf54495618c7e86f432fc1 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Sat, 4 Apr 2020 00:24:49 +0800 Subject: [PATCH 2/3] Add some comments. --- src/graph/CreateEdgeIndexExecutor.cpp | 6 ++++-- src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/graph/CreateEdgeIndexExecutor.cpp b/src/graph/CreateEdgeIndexExecutor.cpp index 796554cf264..bc277b32e89 100644 --- a/src/graph/CreateEdgeIndexExecutor.cpp +++ b/src/graph/CreateEdgeIndexExecutor.cpp @@ -31,14 +31,16 @@ void CreateEdgeIndexExecutor::execute() { auto *edgeName = sentence_->edgeName(); auto columns = sentence_->names(); auto spaceId = ectx()->rctx()->session()->space(); - if (columns.empty()) { + if (UNLIKELY(columns.empty())) { + // It's not allowed by parser in normal + LOG(WARNING) << "Impossible empty index fields."; onError_(Status::MalformedRequest("Empty fields.")); return; } std::unordered_set uniFields; uniFields.reserve(columns.size()); uniFields.insert(columns.begin(), columns.end()); - if (uniFields.size() != columns.size()) { + if (UNLIKELY(uniFields.size() != columns.size())) { onError_(Status::MalformedRequest("Duplicate fields.")); return; } diff --git a/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp b/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp index 335a889ffc0..41beefbfd13 100644 --- a/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp +++ b/src/meta/processors/indexMan/CreateEdgeIndexProcessor.cpp @@ -13,7 +13,7 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq &req) { const auto &indexName = req.get_index_name(); auto &edgeName = req.get_edge_name(); auto &fieldNames = req.get_fields(); - if (fieldNames.empty()) { + if (UNLIKELY(fieldNames.empty())) { LOG(ERROR) << "The index field of an edge type should not be empty."; handleErrorCode(cpp2::ErrorCode::E_INVALID_PARM); onFinished(); @@ -21,7 +21,7 @@ void CreateEdgeIndexProcessor::process(const cpp2::CreateEdgeIndexReq &req) { } std::set columnSet(fieldNames.begin(), fieldNames.end()); - if (fieldNames.size() != columnSet.size()) { + if (UNLIKELY(fieldNames.size() != columnSet.size())) { LOG(ERROR) << "Conflict field in the edge index."; handleErrorCode(cpp2::ErrorCode::E_CONFLICT); onFinished(); From 0e5fd33119aa2be31a5e534585439cc96e717d0f Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Tue, 7 Apr 2020 09:27:16 +0800 Subject: [PATCH 3/3] Using std::move. --- src/graph/CreateEdgeIndexExecutor.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/graph/CreateEdgeIndexExecutor.cpp b/src/graph/CreateEdgeIndexExecutor.cpp index bc277b32e89..7f87ecbd725 100644 --- a/src/graph/CreateEdgeIndexExecutor.cpp +++ b/src/graph/CreateEdgeIndexExecutor.cpp @@ -45,8 +45,8 @@ void CreateEdgeIndexExecutor::execute() { return; } - auto future = - mc->createEdgeIndex(spaceId, *name, *edgeName, columns, sentence_->isIfNotExist()); + auto future = mc->createEdgeIndex( + spaceId, *name, *edgeName, std::move(columns), sentence_->isIfNotExist()); auto *runner = ectx()->rctx()->runner(); auto cb = [this](auto &&resp) { if (!resp.ok()) {