From afbf9d281c4f902be8f47d63df142b1bbcf25f18 Mon Sep 17 00:00:00 2001 From: "lionel.liu@vesoft.com" <52276794+liuyu85cn@users.noreply.github.com> Date: Wed, 13 Oct 2021 10:32:25 +0800 Subject: [PATCH] [two fix] 1. if propNames is empty, replaceDefaultValue wrong. 2. txnMgr may core while shutdown (#3025) Co-authored-by: CBS <56461666+bright-starry-sky@users.noreply.github.com> --- src/storage/StorageServer.cpp | 7 +- .../ChainAddEdgesProcessorLocal.cpp | 112 +++++++++++++----- 2 files changed, 85 insertions(+), 34 deletions(-) diff --git a/src/storage/StorageServer.cpp b/src/storage/StorageServer.cpp index 816b4e46b11..b415f9963b4 100644 --- a/src/storage/StorageServer.cpp +++ b/src/storage/StorageServer.cpp @@ -336,6 +336,9 @@ void StorageServer::stop() { webSvc_.reset(); + if (txnMan_) { + txnMan_->stop(); + } if (taskMgr_) { taskMgr_->shutdown(); } @@ -348,10 +351,6 @@ void StorageServer::stop() { if (adminServer_) { adminServer_->stop(); } - if (txnMan_) { - txnMan_->stop(); - txnMan_.reset(); - } if (internalStorageServer_) { internalStorageServer_->stop(); } diff --git a/src/storage/transaction/ChainAddEdgesProcessorLocal.cpp b/src/storage/transaction/ChainAddEdgesProcessorLocal.cpp index 70317f31235..8ac4d12994a 100644 --- a/src/storage/transaction/ChainAddEdgesProcessorLocal.cpp +++ b/src/storage/transaction/ChainAddEdgesProcessorLocal.cpp @@ -136,7 +136,7 @@ bool ChainAddEdgesProcessorLocal::prepareRequest(const cpp2::AddEdgesRequest& re spaceVidType_ = vidType.value(); } localPartId_ = req.get_parts().begin()->first; - // replaceNullWithDefaultValue(req_); + replaceNullWithDefaultValue(req_); auto part = env_->kvstore_->part(spaceId_, localPartId_); if (!nebula::ok(part)) { pushResultCode(nebula::error(part), localPartId_); @@ -425,44 +425,96 @@ std::string ChainAddEdgesProcessorLocal::makeReadableEdge(const cpp2::AddEdgesRe * in/out edge, but they will calculate independent * which lead to inconsistance * - * that why we need to replace the inconsistance prone value + * that's why we need to replace the inconsistance prone value * at the monment the request comes * */ void ChainAddEdgesProcessorLocal::replaceNullWithDefaultValue(cpp2::AddEdgesRequest& req) { + auto& edgesOfPart = *req.parts_ref(); + if (edgesOfPart.empty()) { + return; + } + auto& edgesOfFirstPart = edgesOfPart.begin()->second; + if (edgesOfFirstPart.empty()) { + return; + } + auto firstEdgeKey = edgesOfFirstPart.front().get_key(); + auto edgeType = std::abs(*firstEdgeKey.edge_type_ref()); + auto schema = env_->schemaMan_->getEdgeSchema(spaceId_, edgeType); + DefaultValueContext expCtx; + // the coming request has two forms + // 1st "propNames" is empty, + // which means all vals should be write as the same sequence of schema + // 2nd "propNames" is not empty + // vals of request should be write according to propName of schema + // use the following "idxVec" to identify which index a val should be write to. + std::vector idxVec; auto& propNames = *req.prop_names_ref(); + if (propNames.empty()) { + for (auto i = 0U; i < schema->getNumFields(); ++i) { + idxVec.emplace_back(i); + } + } else { + // first scan the origin input propNames + for (auto& name : propNames) { + int64_t index = schema->getFieldIndex(name); + idxVec.emplace_back(index); + } + // second, check if there any cols not filled but has default val + // we need to append these cols + for (auto i = 0U; i < schema->getNumFields(); ++i) { + auto it = std::find(idxVec.begin(), idxVec.end(), i); + if (it == idxVec.end()) { + auto field = schema->field(i); + if (field->hasDefault()) { + idxVec.emplace_back(i); + } + } + } + } + for (auto& part : *req.parts_ref()) { for (auto& edge : part.second) { - auto edgeKey = edge.get_key(); - auto edgeType = std::abs(*edgeKey.edge_type_ref()); - auto schema = env_->schemaMan_->getEdgeSchema(spaceId_, edgeType); auto& vals = *edge.props_ref(); - for (auto i = 0U; i < schema->getNumFields(); ++i) { - std::string fieldName(schema->getFieldName(i)); - auto it = std::find(propNames.begin(), propNames.end(), fieldName); - if (it == propNames.end()) { - auto field = schema->field(i); - if (field->hasDefault()) { - auto expr = field->defaultValue()->clone(); - propNames.emplace_back(fieldName); - auto defVal = Expression::eval(expr, expCtx); - switch (defVal.type()) { - case Value::Type::DATE: - vals.emplace_back(defVal.getDate()); - break; - case Value::Type::TIME: - vals.emplace_back(defVal.getTime()); - break; - case Value::Type::DATETIME: - vals.emplace_back(defVal.getDateTime()); - break; - default: - // for other type, local and remote should behavior same. - break; - } - } else { - // it's ok if this field doesn't have a default value + if (vals.size() > idxVec.size()) { + LOG(ERROR) << folly::sformat( + "error vals.size()={} > idxVec.size()={}", vals.size(), idxVec.size()); + continue; + } + for (auto i = vals.size(); i < idxVec.size(); ++i) { + auto field = schema->field(idxVec[i]); + if (field->hasDefault()) { + auto expr = field->defaultValue()->clone(); + auto defVal = Expression::eval(expr, expCtx); + switch (defVal.type()) { + case Value::Type::BOOL: + vals.emplace_back(defVal.getBool()); + break; + case Value::Type::INT: + vals.emplace_back(defVal.getInt()); + break; + case Value::Type::FLOAT: + vals.emplace_back(defVal.getFloat()); + break; + case Value::Type::STRING: + vals.emplace_back(defVal.getStr()); + break; + case Value::Type::DATE: + vals.emplace_back(defVal.getDate()); + break; + case Value::Type::TIME: + vals.emplace_back(defVal.getTime()); + break; + case Value::Type::DATETIME: + vals.emplace_back(defVal.getDateTime()); + break; + default: + // for other type, local and remote should behavior same. + break; } + } else { + // set null + vals.emplace_back(Value::kNullValue); } } }