Skip to content

Commit

Permalink
Fixed graphd crash (#1403)
Browse files Browse the repository at this point in the history 
* Issue #1364 Fixed crash caused when not specifying edge type (over *) and setting reversely.

* address CPWstatic's comments.

* 1 address laura's comments.
2 Fixed a bug in fetching the shortest path (when there are different types of edges between two points will cause one of them to be ignored).
  • Loading branch information
@monadbobo @dutor
monadbobo authored and dutor committed Dec 19, 2019
1 parent 446561e commit 6669a45
Show file tree
Hide file tree
Showing 9 changed files with 314 additions and 43 deletions.
4 changes: 4 additions & 0 deletions src/client/cpp/GraphClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ cpp2::ErrorCode GraphClient::execute(folly::StringPiece stmt,
return cpp2::ErrorCode::E_RPC_FAILURE;
}

auto* msg = resp.get_error_msg();
if (msg != nullptr) {
LOG(WARNING) << *msg;
}
return resp.get_error_code();
}

Expand Down
8 changes: 5 additions & 3 deletions src/daemons/MetaDaemon.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ using nebula::Status;
DEFINE_int32(port, 45500, "Meta daemon listening port");
DEFINE_bool(reuse_port, true, "Whether to turn on the SO_REUSEPORT option");
DEFINE_string(data_path, "", "Root data path");
DEFINE_string(meta_server_addrs, "", "It is a list of IPs split by comma, used in cluster deployment"
"the ips number is equal to the replica number."
"If empty, it means it's a single node");
DEFINE_string(meta_server_addrs,
"",
"It is a list of IPs split by comma, used in cluster deployment"
"the ips number is equal to the replica number."
"If empty, it means it's a single node");
DEFINE_string(local_ip, "", "Local ip specified for NetworkUtils::getLocalIP");
DEFINE_int32(num_io_threads, 16, "Number of IO threads");
DEFINE_int32(meta_http_thread_num, 3, "Number of meta daemon's http thread");
Expand Down
14 changes: 8 additions & 6 deletions src/graph/FindPathExecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,18 +312,19 @@ inline void FindPathExecutor::meetOddPath(VertexID src, VertexID dst, Neighbor &
VLOG(2) << "PathT: " << buildPathString(j->second);

auto target = std::get<0>(*(path.back()));
auto pathTarget = std::get<0>(*(path.back())) + std::get<1>(*(path.back()));
if (shortest_) {
targetNotFound_.erase(target);
if (finalPath_.count(target) > 0) {
if (finalPath_.count(pathTarget) > 0) {
// already found a shorter path
continue;
} else {
VLOG(2) << "Found path: " << buildPathString(path);
finalPath_.emplace(target, std::move(path));
finalPath_.emplace(std::move(pathTarget), std::move(path));
}
} else {
VLOG(2) << "Found path: " << buildPathString(path);
finalPath_.emplace(target, std::move(path));
finalPath_.emplace(std::move(pathTarget), std::move(path));
}
} // for `j'
} // for `i'
Expand Down Expand Up @@ -361,18 +362,19 @@ inline void FindPathExecutor::meetEvenPath(VertexID intersectId) {
VLOG(2) << "PathT: " << buildPathString(j->second);
path.insert(path.end(), j->second.begin(), j->second.end());
auto target = std::get<0>(*(path.back()));
auto pathTarget = std::get<0>(*(path.back())) + std::get<1>(*(path.back()));
if (shortest_) {
if (finalPath_.count(target) > 0) {
if (finalPath_.count(pathTarget) > 0) {
// already found a shorter path
continue;
} else {
targetNotFound_.erase(target);
VLOG(2) << "Found path: " << buildPathString(path);
finalPath_.emplace(target, std::move(path));
finalPath_.emplace(std::move(pathTarget), std::move(path));
}
} else {
VLOG(2) << "Found path: " << buildPathString(path);
finalPath_.emplace(target, std::move(path));
finalPath_.emplace(std::move(pathTarget), std::move(path));
}
}
}
Expand Down
46 changes: 29 additions & 17 deletions src/graph/GoExecutor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ Status GoExecutor::prepareOverAll() {
}

auto v = edgeStatus.value();
if (isReversely()) {
v = -v;
}

edgeTypes_.push_back(v);

if (!expCtx_->addEdge(e, v)) {
Expand Down Expand Up @@ -499,7 +503,8 @@ void GoExecutor::maybeFinishExecution(RpcResponse &&rpcResp) {
// Or, Reversely traversal but no properties on edge and destination nodes required.
// Note that the `dest` which used in reversely traversal means the `src` in foword edge.
if ((!requireDstProps && !isReversely()) ||
(isReversely() && !requireDstProps && !requireEdgeProps)) {
(isReversely() && !requireDstProps && !requireEdgeProps &&
!(expCtx_->isOverAllEdge() && yields_.empty()))) {
finishExecution(std::move(rpcResp));
return;
}
Expand Down Expand Up @@ -560,7 +565,7 @@ void GoExecutor::maybeFinishExecution(RpcResponse &&rpcResp) {
ectx()->getGraphStats()->getGoStats());
return;
}
auto type = edge.type > 0 ? edge.type : -edge.type;
auto type = std::abs(edge.type);
auto &edgeKeys = edgeKeysMapping[type];
edgeKeys.emplace_back();
edgeKeys.back().set_src(dst);
Expand All @@ -581,8 +586,7 @@ void GoExecutor::maybeFinishExecution(RpcResponse &&rpcResp) {
return;
}

edgeType = edgeType > 0 ? edgeType : -edgeType;

edgeType = std::abs(edgeType);
auto &edgeProps = edgePropsMapping[edgeType];
edgeProps.emplace_back();
edgeProps.back().owner = storage::cpp2::PropOwner::EDGE;
Expand Down Expand Up @@ -641,7 +645,6 @@ void GoExecutor::maybeFinishExecution(RpcResponse &&rpcResp) {
folly::collectAll(std::move(futures)).via(runner).thenValue(cb).thenError(error);
}


void GoExecutor::onVertexProps(RpcResponse &&rpcResp) {
UNUSED(rpcResp);
}
Expand All @@ -657,7 +660,7 @@ std::vector<std::string> GoExecutor::getEdgeNamesFromResp(RpcResponse &rpcResp)

for (auto &schema : *edgeSchema) {
auto edgeType = schema.first;
auto status = ectx()->schemaManager()->toEdgeName(spaceId, edgeType);
auto status = ectx()->schemaManager()->toEdgeName(spaceId, std::abs(edgeType));
DCHECK(status.ok());
auto edgeName = status.value();
names.emplace_back(std::move(edgeName));
Expand Down Expand Up @@ -1047,19 +1050,20 @@ bool GoExecutor::processFinalResult(RpcResponse &rpcResp, Callback cb) const {
colTypes.back() = edgeHolder_->getType(
boost::get<VertexID>(value(dst)),
srcid,
edgeType > 0 ? edgeType : -edgeType, prop);
std::abs(edgeType), prop);
}
if (edgeType != type && edgeType != -type) {
return edgeHolder_->getDefaultProp(-type, prop);
if (std::abs(edgeType) != std::abs(type)) {
return edgeHolder_->getDefaultProp(std::abs(type), prop);
}

return edgeHolder_->get(boost::get<VertexID>(value(dst)),
srcid,
edgeType > 0 ? edgeType : -edgeType, prop);
std::abs(edgeType), prop);
} else {
if (saveTypeFlag) {
colTypes.back() = iter->getSchema()->getFieldType(prop).type;
}
if (edgeType != type && edgeType != -type) {
if (std::abs(edgeType) != std::abs(type)) {
auto sit = edgeSchema.find(type);
if (sit == edgeSchema.end()) {
return Status::Error("get schema failed");
Expand Down Expand Up @@ -1289,10 +1293,9 @@ Status GoExecutor::EdgeHolder::add(const storage::cpp2::EdgePropResponse &resp)
++iter;
continue;
}
EdgeKey key;
std::get<0>(key) = boost::get<int64_t>(src.value());
std::get<1>(key) = boost::get<int64_t>(dst.value());
std::get<2>(key) = boost::get<int64_t>(type.value());
auto key = std::make_tuple(boost::get<int64_t>(src.value()),
boost::get<int64_t>(dst.value()),
boost::get<int64_t>(type.value()));
RowWriter rWriter(eschema);
auto fields = iter->numFields();
for (auto i = 0; i < fields; i++) {
Expand All @@ -1302,7 +1305,11 @@ Status GoExecutor::EdgeHolder::add(const storage::cpp2::EdgePropResponse &resp)
}
collector->collect(value(result), &rWriter);
}
edges_.emplace(key, std::make_pair(eschema, rWriter.encode()));

VLOG(2) << "EdgeHolder added edge, type: " << type.value() << " src: " << src.value()
<< " dst: " << dst.value();
edges_.emplace(std::move(key), std::make_pair(eschema, rWriter.encode()));

schemas_.emplace(boost::get<int64_t>(type.value()), eschema);
++iter;
}
Expand Down Expand Up @@ -1340,7 +1347,12 @@ OptVariantType GoExecutor::EdgeHolder::getDefaultProp(EdgeType type,
const std::string &prop) {
auto sit = schemas_.find(type);
if (sit == schemas_.end()) {
return Status::Error("Get default prop failed in reversely traversal.");
// This means that the reversely edge does not exist
if (prop == _DST || prop == _SRC || prop == _RANK) {
return static_cast<int64_t>(0);
} else {
return Status::Error("Get default prop failed in reversely traversal.");
}
}

return RowReader::getDefaultProp(sit->second.get(), prop);
Expand Down
13 changes: 8 additions & 5 deletions src/graph/test/FindPathTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ TEST_F(FindPathTest, MultiEdgesShortest) {
ASSERT_EQ(cpp2::ErrorCode::SUCCEEDED, code) << *(resp.get_error_msg());
std::vector<std::string> expected = {
"5662213458193308137<like,0>-7579316172763586624",
"5662213458193308137<teammate,0>-7579316172763586624"
};
ASSERT_TRUE(verifyPath(resp, expected));
}
Expand All @@ -330,6 +331,7 @@ TEST_F(FindPathTest, MultiEdgesShortest) {
std::vector<std::string> expected = {
"5662213458193308137<like,0>-7579316172763586624",
"5662213458193308137<serve,0>7193291116733635180",
"5662213458193308137<teammate,0>-7579316172763586624",
};
ASSERT_TRUE(verifyPath(resp, expected));
}
Expand All @@ -347,10 +349,12 @@ TEST_F(FindPathTest, MultiEdgesShortest) {
auto code = client_->execute(query, resp);
ASSERT_EQ(cpp2::ErrorCode::SUCCEEDED, code) << *(resp.get_error_msg());
std::vector<std::string> expected = {
"5662213458193308137<like,0>3394245602834314645",
"5662213458193308137<like,0>-7579316172763586624",
"5662213458193308137<like,0>-7579316172763586624<like,0>-1782445125509592239",
"5662213458193308137<like,0>3394245602834314645",
"5662213458193308137<serve,0>7193291116733635180",
"5662213458193308137<teammate,0>-1782445125509592239",
"5662213458193308137<teammate,0>-7579316172763586624",
"5662213458193308137<teammate,0>3394245602834314645"
};
ASSERT_TRUE(verifyPath(resp, expected));
}
Expand All @@ -363,8 +367,7 @@ TEST_F(FindPathTest, MultiEdgesShortest) {
auto code = client_->execute(query, resp);
ASSERT_EQ(cpp2::ErrorCode::SUCCEEDED, code) << *(resp.get_error_msg());
std::vector<std::string> expected = {
"-8160811731890648949<like,0>5662213458193308137<like,0>-7579316172763586624"
"<like,0>-1782445125509592239",
"-8160811731890648949<like,0>5662213458193308137<teammate,0>-1782445125509592239",
"-8160811731890648949<serve,0>7193291116733635180",
};
ASSERT_TRUE(verifyPath(resp, expected));
Expand Down Expand Up @@ -408,7 +411,7 @@ TEST_F(FindPathTest, MultiEdgesAll) {
}
{
cpp2::ExecutionResponse resp;
auto *fmt = "FIND ALL PATH FROM %ld TO %ld,%ld OVER * UPTO 3 STEPS";
auto *fmt = "FIND ALL PATH FROM %ld TO %ld,%ld OVER like,serve UPTO 3 STEPS";
auto &tim = players_["Tim Duncan"];
auto &tony = players_["Tony Parker"];
auto query = folly::stringPrintf(fmt, tim.vid(), tony.vid(), teams_["Spurs"].vid());
Expand Down
Loading

0 comments on commit 6669a45

Please sign in to comment.