diff --git a/src/query/v2/requests.hpp b/src/query/v2/requests.hpp index 62a4e93f8..f61275377 100644 --- a/src/query/v2/requests.hpp +++ b/src/query/v2/requests.hpp @@ -367,7 +367,6 @@ struct ScanResultRow { }; struct ScanVerticesResponse { - bool success; std::optional error; std::optional next_start_id; std::vector results; @@ -388,7 +387,6 @@ struct GetPropertiesRequest { }; struct GetPropertiesResponse { - bool success; std::optional error; }; @@ -454,7 +452,6 @@ struct ExpandOneResultRow { }; struct ExpandOneResponse { - bool success; std::optional error; std::vector result; }; @@ -489,7 +486,6 @@ struct CreateVerticesRequest { }; struct CreateVerticesResponse { - bool success; std::optional error; }; @@ -501,7 +497,6 @@ struct DeleteVerticesRequest { }; struct DeleteVerticesResponse { - bool success; std::optional error; }; @@ -511,7 +506,6 @@ struct UpdateVerticesRequest { }; struct UpdateVerticesResponse { - bool success; std::optional error; }; @@ -534,7 +528,6 @@ struct CreateExpandRequest { }; struct CreateExpandResponse { - bool success; std::optional error; }; @@ -544,7 +537,6 @@ struct DeleteEdgesRequest { }; struct DeleteEdgesResponse { - bool success; std::optional error; }; @@ -554,7 +546,6 @@ struct UpdateEdgesRequest { }; struct UpdateEdgesResponse { - bool success; std::optional error; }; @@ -564,7 +555,6 @@ struct CommitRequest { }; struct CommitResponse { - bool success; std::optional error; }; diff --git a/src/query/v2/shard_request_manager.hpp b/src/query/v2/shard_request_manager.hpp index a73201046..20bae7b97 100644 --- a/src/query/v2/shard_request_manager.hpp +++ b/src/query/v2/shard_request_manager.hpp @@ -206,7 +206,7 @@ class ShardRequestManager : public ShardRequestManagerInterface { } WriteResponses write_response_variant = commit_response.GetValue(); auto &response = std::get(write_response_variant); - if (!response.success) { + if (response.error) { throw std::runtime_error("Commit request did not succeed"); } } @@ -311,7 +311,7 @@ class ShardRequestManager : public ShardRequestManagerInterface { WriteResponses response_variant = write_response_result.GetValue(); CreateExpandResponse mapped_response = std::get(response_variant); - if (!mapped_response.success) { + if (mapped_response.error) { throw std::runtime_error("CreateExpand request did not succeed"); } responses.push_back(mapped_response); @@ -601,7 +601,7 @@ class ShardRequestManager : public ShardRequestManagerInterface { WriteResponses response_variant = poll_result->GetValue(); auto response = std::get(response_variant); - if (!response.success) { + if (response.error) { throw std::runtime_error("CreateVertices request did not succeed"); } responses.push_back(response); @@ -637,7 +637,7 @@ class ShardRequestManager : public ShardRequestManagerInterface { // Currently a boolean flag for signaling the overall success of the // ExpandOne request does not exist. But it should, so here we assume // that it is already in place. - if (!response.success) { + if (response.error) { throw std::runtime_error("ExpandOne request did not succeed"); } @@ -680,7 +680,7 @@ class ShardRequestManager : public ShardRequestManagerInterface { ReadResponses read_response_variant = await_result->GetValue(); auto response = std::get(read_response_variant); - if (!response.success) { + if (response.error) { throw std::runtime_error("ScanAll request did not succeed"); } diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp index 1ff8e04b7..7a128a78f 100644 --- a/src/storage/v3/shard_rsm.cpp +++ b/src/storage/v3/shard_rsm.cpp @@ -487,7 +487,6 @@ auto CreateErrorResponse(const ShardError &shard_error, const auto transaction_i msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateVerticesRequest &&req) { auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; for (auto &new_vertex : req.new_vertices) { @@ -510,28 +509,25 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateVerticesRequest &&req) { if (result_schema.HasError()) { shard_error.emplace(CreateErrorResponse(result_schema.GetError(), req.transaction_id, "creating vertices")); - action_successful = false; break; } } - return msgs::CreateVerticesResponse{action_successful, std::move(shard_error)}; + return msgs::CreateVerticesResponse{std::move(shard_error)}; } msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateVerticesRequest &&req) { auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; for (auto &vertex : req.new_properties) { - if (!action_successful) { + if (shard_error) { break; } auto vertex_to_update = acc.FindVertex(ConvertPropertyVector(std::move(vertex.primary_key)), View::OLD); if (!vertex_to_update) { - action_successful = false; shard_error.emplace(msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND}); spdlog::debug("In transaction {} vertex could not be found while trying to update its properties.", req.transaction_id.logical_id); @@ -542,18 +538,16 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateVerticesRequest &&req) { auto result_schema = vertex_to_update->SetPropertyAndValidate(update_prop.first, ToPropertyValue(std::move(update_prop.second))); if (result_schema.HasError()) { - action_successful = false; shard_error.emplace(CreateErrorResponse(result_schema.GetError(), req.transaction_id, "updating vertices")); break; } } } - return msgs::UpdateVerticesResponse{action_successful, std::move(shard_error)}; + return msgs::UpdateVerticesResponse{std::move(shard_error)}; } msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteVerticesRequest &&req) { - bool action_successful = true; std::optional shard_error; auto acc = shard_->Access(req.transaction_id); @@ -563,7 +557,6 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteVerticesRequest &&req) { if (!vertex_acc) { spdlog::debug("Error while trying to delete vertex. Vertex to delete does not exist. Transaction id: {}", req.transaction_id.logical_id); - action_successful = false; shard_error.emplace(msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND}); spdlog::debug("In transaction {} vertex could not be found while trying to delete it.", req.transaction_id.logical_id); @@ -576,7 +569,6 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteVerticesRequest &&req) { case msgs::DeleteVerticesRequest::DeletionType::DELETE: { auto result = acc.DeleteVertex(&vertex_acc.value()); if (result.HasError() || !(result.GetValue().has_value())) { - action_successful = false; shard_error.emplace(CreateErrorResponse(result.GetError(), req.transaction_id, "deleting vertices")); } break; @@ -584,23 +576,21 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteVerticesRequest &&req) { case msgs::DeleteVerticesRequest::DeletionType::DETACH_DELETE: { auto result = acc.DetachDeleteVertex(&vertex_acc.value()); if (result.HasError() || !(result.GetValue().has_value())) { - action_successful = false; shard_error.emplace(CreateErrorResponse(result.GetError(), req.transaction_id, "deleting vertices")); } break; } } - if (!action_successful) { + if (shard_error) { break; } } - return msgs::DeleteVerticesResponse{action_successful, std::move(shard_error)}; + return msgs::DeleteVerticesResponse{std::move(shard_error)}; } msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateExpandRequest &&req) { auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; for (auto &new_expand : req.new_expands) { @@ -611,7 +601,6 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateExpandRequest &&req) { VertexId{new_expand.dest_vertex.first.id, ConvertPropertyVector(std::move(new_expand.dest_vertex.second))}; if (!(shard_->IsVertexBelongToShard(from_vertex_id) || shard_->IsVertexBelongToShard(to_vertex_id))) { - action_successful = false; shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Error while trying to insert edge, none of the vertices belong to this shard"}; spdlog::debug("Error while trying to insert edge, none of the vertices belong to this shard. Transaction id: {}", @@ -625,19 +614,17 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateExpandRequest &&req) { if (!new_expand.properties.empty()) { for (const auto &[property, value] : new_expand.properties) { if (const auto maybe_error = edge.SetProperty(property, ToPropertyValue(value)); maybe_error.HasError()) { - action_successful = false; shard_error.emplace( CreateErrorResponse(maybe_error.GetError(), req.transaction_id, "setting edge property")); break; } } - if (!action_successful) { + if (shard_error) { break; } } } else { // TODO Code for this - action_successful = false; shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND}; spdlog::debug("Creating edge was not successful. Transaction id: {}", req.transaction_id.logical_id); break; @@ -648,7 +635,6 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateExpandRequest &&req) { for (auto &[edge_prop_key, edge_prop_val] : new_expand.properties) { auto set_result = edge_acc->SetProperty(edge_prop_key, ToPropertyValue(std::move(edge_prop_val))); if (set_result.HasError()) { - action_successful = false; shard_error.emplace(CreateErrorResponse(set_result.GetError(), req.transaction_id, "adding edge property")); break; } @@ -656,16 +642,15 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CreateExpandRequest &&req) { } } - return msgs::CreateExpandResponse{action_successful, std::move(shard_error)}; + return msgs::CreateExpandResponse{std::move(shard_error)}; } msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteEdgesRequest &&req) { - bool action_successful = true; std::optional shard_error; auto acc = shard_->Access(req.transaction_id); for (auto &edge : req.edges) { - if (!action_successful) { + if (shard_error) { break; } @@ -673,30 +658,27 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::DeleteEdgesRequest &&req) { VertexId(edge.dst.first.id, ConvertPropertyVector(std::move(edge.dst.second))), Gid::FromUint(edge.id.gid)); if (edge_acc.HasError() || !edge_acc.HasValue()) { - action_successful = false; shard_error.emplace(CreateErrorResponse(edge_acc.GetError(), req.transaction_id, "delete edge")); continue; } } - return msgs::DeleteEdgesResponse{action_successful, std::move(shard_error)}; + return msgs::DeleteEdgesResponse{std::move(shard_error)}; } msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateEdgesRequest &&req) { // TODO(antaljanosbenjamin): handle when the vertex is the destination vertex auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; for (auto &edge : req.new_properties) { - if (!action_successful) { + if (shard_error) { break; } auto vertex_acc = acc.FindVertex(ConvertPropertyVector(std::move(edge.src.second)), View::OLD); if (!vertex_acc) { - action_successful = false; shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Source vertex was not found"}; spdlog::debug("Encountered an error while trying to acquire VertexAccessor with transaction id: {}", req.transaction_id.logical_id); @@ -707,7 +689,6 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateEdgesRequest &&req) { // in the vertex's out-going edges auto edges_res = vertex_acc->OutEdges(View::OLD); if (edges_res.HasError()) { - action_successful = false; shard_error.emplace(CreateErrorResponse(edges_res.GetError(), req.transaction_id, "update edge")); continue; } @@ -734,19 +715,17 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::UpdateEdgesRequest &&req) { if (!edge_accessor_did_match) { // TODO(jbajic) Do we need this shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Edge was not found"}; - action_successful = false; spdlog::debug("Could not find the Edge with the specified Gid. Transaction id: {}", req.transaction_id.logical_id); continue; } } - return msgs::UpdateEdgesResponse{action_successful, std::move(shard_error)}; + return msgs::UpdateEdgesResponse{std::move(shard_error)}; } msgs::ReadResponses ShardRsm::HandleRead(msgs::ScanVerticesRequest &&req) { auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; std::vector results; @@ -786,7 +765,6 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ScanVerticesRequest &&req) { // Is it useful to return just a vertex without the properties? if (!found_props) { shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Requested properties were not found!"}; - action_successful = false; } results.emplace_back(msgs::ScanResultRow{.vertex = ConstructValueVertex(vertex, view).vertex_v, @@ -834,9 +812,8 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ScanVerticesRequest &&req) { } } - msgs::ScanVerticesResponse resp{.success = action_successful, .error = std::move(shard_error)}; - - if (action_successful) { + msgs::ScanVerticesResponse resp{.error = std::move(shard_error)}; + if (resp.error) { resp.next_start_id = next_start_id; resp.results = std::move(results); } @@ -846,7 +823,6 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ScanVerticesRequest &&req) { msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { auto acc = shard_->Access(req.transaction_id); - bool action_successful = true; std::optional shard_error; std::vector results; @@ -859,7 +835,6 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { auto src_vertex_acc_opt = acc.FindVertex(ConvertPropertyVector((src_vertex.second)), View::NEW); if (!src_vertex_acc_opt) { shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Source vertex was not found."}; - action_successful = false; spdlog::debug("Encountered an error while trying to obtain VertexAccessor. Transaction id: {}", req.transaction_id.logical_id); break; @@ -879,15 +854,14 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { if (result.HasError()) { // Code Error shard_error = msgs::ShardError{common::ErrorCode::OBJECT_NOT_FOUND, "Source vertex was not found."}; - action_successful = false; break; } results.emplace_back(result.GetValue()); } - msgs::ExpandOneResponse resp{.success = action_successful, .error = std::move(shard_error)}; - if (action_successful) { + msgs::ExpandOneResponse resp{.error = std::move(shard_error)}; + if (!resp.error) { resp.result = std::move(results); } @@ -896,7 +870,7 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::ExpandOneRequest &&req) { msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CommitRequest &&req) { shard_->Access(req.transaction_id).Commit(req.commit_timestamp); - return msgs::CommitResponse{true}; + return msgs::CommitResponse{}; }; // NOLINTNEXTLINE(readability-convert-member-functions-to-static) diff --git a/tests/simulation/shard_rsm.cpp b/tests/simulation/shard_rsm.cpp index 64d0a0861..b9511f024 100644 --- a/tests/simulation/shard_rsm.cpp +++ b/tests/simulation/shard_rsm.cpp @@ -137,7 +137,7 @@ void Commit(ShardClient &client, const coordinator::Hlc &transaction_timestamp) auto write_response_result = write_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success, "Commit expected to be successful, but it is failed"); + MG_ASSERT(!write_response.error.has_value(), "Commit expected to be successful, but it is failed"); break; } @@ -156,7 +156,7 @@ bool AttemptToCreateVertex(ShardClient &client, int64_t value) { create_req.transaction_id.logical_id = GetTransactionId(); auto write_res = client.SendWriteRequest(create_req); - MG_ASSERT(write_res.HasValue() && std::get(write_res.GetValue()).success, + MG_ASSERT(write_res.HasValue() && std::get(write_res.GetValue()).error, "Unexpected failure"); Commit(client, create_req.transaction_id); @@ -179,7 +179,7 @@ bool AttemptToDeleteVertex(ShardClient &client, int64_t value) { auto write_response = std::get(write_response_result); Commit(client, delete_req.transaction_id); - return write_response.success; + return !write_response.error.has_value(); } } @@ -207,7 +207,7 @@ bool AttemptToUpdateVertex(ShardClient &client, int64_t value) { auto write_response = std::get(write_response_result); Commit(client, update_req.transaction_id); - return write_response.success; + return !write_response.error.has_value(); } } @@ -244,7 +244,7 @@ bool AttemptToAddEdge(ShardClient &client, int64_t value_of_vertex_1, int64_t va Commit(client, create_req.transaction_id); - return write_response.success; + return !write_response.error.has_value(); } return true; } @@ -276,7 +276,7 @@ bool AttemptToAddEdgeWithProperties(ShardClient &client, int64_t value_of_vertex create_req.transaction_id.logical_id = GetTransactionId(); auto write_res = client.SendWriteRequest(create_req); - MG_ASSERT(write_res.HasValue() && std::get(write_res.GetValue()).success, + MG_ASSERT(write_res.HasValue() && !std::get(write_res.GetValue()).error.has_value(), "Unexpected failure"); Commit(client, create_req.transaction_id); @@ -316,7 +316,7 @@ bool AttemptToDeleteEdge(ShardClient &client, int64_t value_of_vertex_1, int64_t auto write_response = std::get(write_response_result); Commit(client, delete_req.transaction_id); - return write_response.success; + return !write_response.error.has_value(); } } @@ -356,7 +356,7 @@ bool AttemptToUpdateEdge(ShardClient &client, int64_t value_of_vertex_1, int64_t auto write_response = std::get(write_response_result); Commit(client, update_req.transaction_id); - return write_response.success; + return !write_response.error.has_value(); } } @@ -379,7 +379,7 @@ std::tuple> AttemptToScanAllWithoutBatchLi auto write_response_result = read_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success); + MG_ASSERT(write_response.error == std::nullopt); return {write_response.results.size(), write_response.next_start_id}; } @@ -405,7 +405,7 @@ std::tuple> AttemptToScanAllWithBatchLimit auto write_response_result = read_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success); + MG_ASSERT(!write_response.error.has_value()); return {write_response.results.size(), write_response.next_start_id}; } @@ -439,7 +439,7 @@ std::tuple> AttemptToScanAllWithExpression auto write_response_result = read_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success); + MG_ASSERT(!write_response.error.has_value()); MG_ASSERT(!write_response.results.empty(), "There are no results!"); MG_ASSERT(write_response.results[0].evaluated_vertex_expressions[0].int_v == 4); return {write_response.results.size(), write_response.next_start_id}; @@ -464,7 +464,7 @@ void AttemptToScanAllWithOrderByOnPrimaryProperty(ShardClient &client, msgs::Ver auto write_response_result = read_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success); + MG_ASSERT(!write_response.error.has_value()); MG_ASSERT(write_response.results.size() == 5, "Expecting 5 results!"); for (int64_t i{0}; i < 5; ++i) { const auto expected_primary_key = std::vector{msgs::Value(1023 - i)}; @@ -494,7 +494,7 @@ void AttemptToScanAllWithOrderByOnSecondaryProperty(ShardClient &client, msgs::V auto write_response_result = read_res.GetValue(); auto write_response = std::get(write_response_result); - MG_ASSERT(write_response.success); + MG_ASSERT(!write_response.error.has_value()); MG_ASSERT(write_response.results.size() == 5, "Expecting 5 results!"); for (int64_t i{0}; i < 5; ++i) { const auto expected_prop4 = std::vector{msgs::Value(1023 - i)}; diff --git a/tests/simulation/test_cluster.hpp b/tests/simulation/test_cluster.hpp index 6a32a391d..ce304d1cc 100644 --- a/tests/simulation/test_cluster.hpp +++ b/tests/simulation/test_cluster.hpp @@ -173,7 +173,7 @@ void ExecuteOp(msgs::ShardRequestManager &shard_request_mana auto result = shard_request_manager.Request(state, std::move(new_vertices)); RC_ASSERT(result.size() == 1); - RC_ASSERT(result[0].success); + RC_ASSERT(!result[0].error.has_value()); correctness_model.emplace(std::make_pair(create_vertex.first, create_vertex.second)); } diff --git a/tests/unit/high_density_shard_create_scan.cpp b/tests/unit/high_density_shard_create_scan.cpp index 9c2d1cfd7..a0c0a0c28 100644 --- a/tests/unit/high_density_shard_create_scan.cpp +++ b/tests/unit/high_density_shard_create_scan.cpp @@ -187,7 +187,7 @@ void ExecuteOp(msgs::ShardRequestManager &shard_request_manager, auto result = shard_request_manager.Request(state, std::move(new_vertices)); MG_ASSERT(result.size() == 1); - MG_ASSERT(result[0].success); + MG_ASSERT(!result[0].error.has_value()); correctness_model.emplace(std::make_pair(create_vertex.first, create_vertex.second)); } diff --git a/tests/unit/machine_manager.cpp b/tests/unit/machine_manager.cpp index 1d69da5c5..7b57d61ea 100644 --- a/tests/unit/machine_manager.cpp +++ b/tests/unit/machine_manager.cpp @@ -151,7 +151,7 @@ void TestCreateExpand(msgs::ShardRequestManagerInterface &shard_request_manager) auto responses = shard_request_manager.Request(state, std::move(new_expands)); MG_ASSERT(responses.size() == 1); - MG_ASSERT(responses[0].success); + MG_ASSERT(!responses[0].error.has_value()); } void TestExpandOne(msgs::ShardRequestManagerInterface &shard_request_manager) {