From d48fff70f2401cd4d787fa4fc9223d26206a08cd Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 29 Oct 2019 20:36:06 +0100 Subject: [PATCH] fix some tests, and remove legacy API response --- arangod/Aql/AqlItemBlock.cpp | 2 -- arangod/Aql/DistributeConsumerNode.h | 4 +-- .../Aql/EngineInfoContainerCoordinator.cpp | 9 ++++-- arangod/Aql/ExecutionEngine.cpp | 5 ++-- arangod/Aql/InputAqlItemRow.cpp | 2 -- arangod/Aql/RemoteExecutor.cpp | 30 ++++++++++++------- arangod/Aql/RestAqlHandler.cpp | 3 +- arangod/Aql/RestAqlHandler.h | 4 +-- 8 files changed, 34 insertions(+), 25 deletions(-) diff --git a/arangod/Aql/AqlItemBlock.cpp b/arangod/Aql/AqlItemBlock.cpp index d3bd739c5c..faf51f5dd5 100644 --- a/arangod/Aql/AqlItemBlock.cpp +++ b/arangod/Aql/AqlItemBlock.cpp @@ -580,8 +580,6 @@ void AqlItemBlock::toVelocyPack(transaction::Methods* trx, VPackBuilder& result) result.add("nrItems", VPackValue(_nrItems)); result.add("nrRegs", VPackValue(_nrRegs)); result.add("error", VPackValue(false)); - // Backwards compatbility 3.3 - result.add("exhausted", VPackValue(false)); enum State { Empty, // saw an empty value diff --git a/arangod/Aql/DistributeConsumerNode.h b/arangod/Aql/DistributeConsumerNode.h index be58678615..d0d5c2392f 100644 --- a/arangod/Aql/DistributeConsumerNode.h +++ b/arangod/Aql/DistributeConsumerNode.h @@ -80,12 +80,12 @@ class DistributeConsumerNode : public ExecutionNode { // This node is not allowed to be cloned. // Clone specialization! TRI_ASSERT(false); - THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL); + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "DistributeConsumerNode cannot be cloned"); } CostEstimate estimateCost() const override { TRI_ASSERT(false); - THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL); + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "DistributeConsumerNode cannot be estimated"); } void cloneRegisterPlan(ScatterNode* dependency); diff --git a/arangod/Aql/EngineInfoContainerCoordinator.cpp b/arangod/Aql/EngineInfoContainerCoordinator.cpp index f6479f17f8..5c03fb6248 100644 --- a/arangod/Aql/EngineInfoContainerCoordinator.cpp +++ b/arangod/Aql/EngineInfoContainerCoordinator.cpp @@ -88,19 +88,22 @@ Result EngineInfoContainerCoordinator::EngineInfo::buildEngine( // For _id == 0 this thread will always maintain the handle to // the engine and will clean up. We do not keep track of it seperately if (_id != 0) { + coordinatorQueryIds.emplace_back(_id); + double ttl = query.queryOptions().ttl; TRI_ASSERT(ttl > 0); try { queryRegistry->insert(_id, &query, ttl, true, false); } catch (basics::Exception const& e) { + coordinatorQueryIds.pop_back(); return {e.code(), e.message()}; } catch (std::exception const& e) { + coordinatorQueryIds.pop_back(); return {TRI_ERROR_INTERNAL, e.what()}; } catch (...) { - return {TRI_ERROR_INTERNAL}; + coordinatorQueryIds.pop_back(); + return {TRI_ERROR_INTERNAL, "unable to store query in registry"}; } - - coordinatorQueryIds.emplace_back(_id); } return {TRI_ERROR_NO_ERROR}; diff --git a/arangod/Aql/ExecutionEngine.cpp b/arangod/Aql/ExecutionEngine.cpp index f363f5f2ad..c86d667d93 100644 --- a/arangod/Aql/ExecutionEngine.cpp +++ b/arangod/Aql/ExecutionEngine.cpp @@ -589,12 +589,11 @@ std::pair ExecutionEngine::skipSome(size_t atMost) { } Result ExecutionEngine::shutdownSync(int errorCode) noexcept { - Result res{TRI_ERROR_INTERNAL}; + Result res{TRI_ERROR_INTERNAL, "unable to shutdown query"}; ExecutionState state = ExecutionState::WAITING; try { std::shared_ptr sharedState = _query.sharedState(); if (sharedState != nullptr) { - while (state == ExecutionState::WAITING) { std::tie(state, res) = shutdown(errorCode); if (state == ExecutionState::WAITING) { @@ -602,6 +601,8 @@ Result ExecutionEngine::shutdownSync(int errorCode) noexcept { } } } + } catch (std::exception const& ex) { + res.reset(TRI_ERROR_INTERNAL, ex.what()); } catch (...) { res.reset(TRI_ERROR_INTERNAL); } diff --git a/arangod/Aql/InputAqlItemRow.cpp b/arangod/Aql/InputAqlItemRow.cpp index e19867592d..69760279c4 100644 --- a/arangod/Aql/InputAqlItemRow.cpp +++ b/arangod/Aql/InputAqlItemRow.cpp @@ -134,8 +134,6 @@ void InputAqlItemRow::toVelocyPack(transaction::Methods* trx, VPackBuilder& resu result.add("nrItems", VPackValue(1)); result.add("nrRegs", VPackValue(getNrRegisters())); result.add("error", VPackValue(false)); - // Backwards compatbility 3.3 - result.add("exhausted", VPackValue(false)); enum State { Empty, // saw an empty value diff --git a/arangod/Aql/RemoteExecutor.cpp b/arangod/Aql/RemoteExecutor.cpp index aac6e48cdf..be12e6b389 100644 --- a/arangod/Aql/RemoteExecutor.cpp +++ b/arangod/Aql/RemoteExecutor.cpp @@ -104,7 +104,7 @@ std::pair ExecutionBlockImpl ExecutionBlockImpl::initialize InputAqlItemRow const& input) { // For every call we simply forward via HTTP if (_requestInFlight.load(std::memory_order_acquire)) { - return {ExecutionState::WAITING, 0}; + return {ExecutionState::WAITING, TRI_ERROR_NO_ERROR}; } if (!_isResponsibleForInitializeCursor) { @@ -259,11 +259,22 @@ std::pair ExecutionBlockImpl::initialize auto response = std::move(_lastResponse); // Result is the response which is an object containing the ErrorCode + int errorNumber = TRI_ERROR_INTERNAL; // default error code VPackSlice slice = response->slice(); - if (slice.hasKey("code")) { - return {ExecutionState::DONE, slice.get("code").getNumericValue()}; + VPackSlice errorSlice = slice.get(StaticStrings::ErrorNum); + if (!errorSlice.isNumber()) { + errorSlice = slice.get(StaticStrings::Code); } - return {ExecutionState::DONE, TRI_ERROR_INTERNAL}; + if (errorSlice.isNumber()) { + errorNumber = errorSlice.getNumericValue(); + } + + std::string errorMessage; + errorSlice = slice.get(StaticStrings::ErrorMessage); + if (errorSlice.isString()) { + return {ExecutionState::DONE, {errorNumber, errorSlice.copyString()}}; + } + return {ExecutionState::DONE, errorNumber}; } VPackOptions options(VPackOptions::Defaults); @@ -274,13 +285,10 @@ std::pair ExecutionBlockImpl::initialize VPackBuilder builder(buffer, &options); builder.openObject(/*unindexed*/ true); - // Backwards Compatibility 3.3 - // NOTE: Removing this breaks tests in current devel - is this really for - // bc only? - builder.add("exhausted", VPackValue(false)); // Used in 3.4.0 onwards builder.add("done", VPackValue(false)); - builder.add("error", VPackValue(false)); + builder.add(StaticStrings::Code, VPackValue(TRI_ERROR_NO_ERROR)); + builder.add(StaticStrings::Error, VPackValue(false)); // NOTE API change. Before all items have been send. // Now only the one output row is send. builder.add("pos", VPackValue(0)); @@ -338,7 +346,7 @@ std::pair ExecutionBlockImpl::shutdown(i _didReceiveShutdownRequest = true; TRI_ASSERT(_lastResponse == nullptr); - Result res = _lastError; + Result res = std::move(_lastError); _lastError.reset(); if (res.is(TRI_ERROR_QUERY_NOT_FOUND)) { diff --git a/arangod/Aql/RestAqlHandler.cpp b/arangod/Aql/RestAqlHandler.cpp index e1ddfccaf7..2950a5d57d 100644 --- a/arangod/Aql/RestAqlHandler.cpp +++ b/arangod/Aql/RestAqlHandler.cpp @@ -674,6 +674,7 @@ RestStatus RestAqlHandler::handleUseQuery(std::string const& operation, Query* q } // Used in 3.4.0 onwards. answerBuilder.add("done", VPackValue(state == ExecutionState::DONE)); + answerBuilder.add(StaticStrings::Code, VPackValue(TRI_ERROR_NO_ERROR)); if (items.get() == nullptr) { // Backwards Compatibility answerBuilder.add(StaticStrings::Error, VPackValue(false)); @@ -734,7 +735,7 @@ RestStatus RestAqlHandler::handleUseQuery(std::string const& operation, Query* q answerBuilder.add(StaticStrings::Code, VPackValue(res.errorNumber())); } else if (operation == "shutdown") { int errorCode = - VelocyPackHelper::getNumericValue(querySlice, "code", TRI_ERROR_INTERNAL); + VelocyPackHelper::readNumericValue(querySlice, StaticStrings::Code, TRI_ERROR_INTERNAL); ExecutionState state; Result res; diff --git a/arangod/Aql/RestAqlHandler.h b/arangod/Aql/RestAqlHandler.h index a28e98158d..84a003c271 100644 --- a/arangod/Aql/RestAqlHandler.h +++ b/arangod/Aql/RestAqlHandler.h @@ -71,8 +71,8 @@ class RestAqlHandler : public RestVocbaseBaseHandler { // "number": must be a positive integer, the cursor skips as many items, // possibly exhausting the cursor. // The result is a JSON with the attributes "error" (boolean), - // "errorMessage" (if applicable) and "exhausted" (boolean) [3.3 - // and earlier] "done" (boolean) [3.4.0 and later] to indicate + // "errorMessage" (if applicable) and + // "done" (boolean) [3.4.0 and later] to indicate // whether or not the cursor is exhausted. RestStatus useQuery(std::string const& operation, std::string const& idString);