diff options
author | Arne H Juul <arnej@yahooinc.com> | 2021-10-14 10:11:20 +0000 |
---|---|---|
committer | Arne H Juul <arnej@yahooinc.com> | 2021-10-14 10:11:20 +0000 |
commit | df689ff589e185f51dec5a6cfa70e4009ebc176a (patch) | |
tree | b1c55046f2a32f088e2215f4712da41597c473ef | |
parent | adad7cc066e7debe8137737a8ce9c6ce377a2c1a (diff) |
make DocsumReply a proper class
10 files changed, 77 insertions, 59 deletions
diff --git a/searchcore/src/tests/proton/docsummary/docsummary.cpp b/searchcore/src/tests/proton/docsummary/docsummary.cpp index 916b88c19c4..1584cf4cd02 100644 --- a/searchcore/src/tests/proton/docsummary/docsummary.cpp +++ b/searchcore/src/tests/proton/docsummary/docsummary.cpp @@ -324,18 +324,19 @@ void assertTensor(const vespalib::eval::Value::UP & exp, const std::string & fieldName, const DocsumReply & reply, uint32_t id) { + const auto & root = reply.root(); if (exp) { - EXPECT_TRUE(reply.root()["docsums"].valid()); - EXPECT_TRUE(reply.root()["docsums"][id].valid()); - EXPECT_TRUE(reply.root()["docsums"][id]["docsum"].valid()); - EXPECT_TRUE(reply.root()["docsums"][id]["docsum"][fieldName].valid()); - vespalib::Memory data = reply.root()["docsums"][id]["docsum"][fieldName].asData(); + EXPECT_TRUE(root["docsums"].valid()); + EXPECT_TRUE(root["docsums"][id].valid()); + EXPECT_TRUE(root["docsums"][id]["docsum"].valid()); + EXPECT_TRUE(root["docsums"][id]["docsum"][fieldName].valid()); + vespalib::Memory data = root["docsums"][id]["docsum"][fieldName].asData(); vespalib::nbostream x(data.data, data.size); auto tensor = SimpleValue::from_stream(x); EXPECT_TRUE(tensor.get() != nullptr); EXPECT_EQUAL(*exp, *tensor); } else { - EXPECT_FALSE(reply.root()["docsums"][id][fieldName].valid()); + EXPECT_FALSE(root["docsums"][id][fieldName].valid()); } } @@ -343,7 +344,8 @@ bool assertSlime(const std::string &exp, const DocsumReply &reply) { vespalib::Slime expSlime; size_t used = JsonFormat::decode(exp, expSlime); EXPECT_TRUE(used > 0); - return (EXPECT_EQUAL(expSlime.get(), reply.root())); + ASSERT_TRUE(reply.hasSlime()); + return (EXPECT_EQUAL(expSlime, reply.slime())); } TEST_F("requireThatAdapterHandlesAllFieldTypes", Fixture) @@ -564,7 +566,8 @@ TEST("requireThatSummariesTimeout") req.resultClassName = "class2"; req.hits.push_back(DocsumRequest::Hit(gid1)); DocsumReply::UP rep = dc._ddb->getDocsums(req); - const auto & field = rep->root()["errors"]; + const auto & root = rep->root(); + const auto & field = root["errors"]; EXPECT_TRUE(field.valid()); EXPECT_EQUAL(field[0]["type"].asString(), "timeout"); auto bufstring = field[0]["message"].asString(); diff --git a/searchcore/src/tests/proton/summaryengine/summaryengine.cpp b/searchcore/src/tests/proton/summaryengine/summaryengine.cpp index 6660b7bfd55..ca4f2329c19 100644 --- a/searchcore/src/tests/proton/summaryengine/summaryengine.cpp +++ b/searchcore/src/tests/proton/summaryengine/summaryengine.cpp @@ -135,7 +135,8 @@ void assertSlime(const std::string &exp, const DocsumReply &reply) { vespalib::Slime expSlime; size_t used = JsonFormat::decode(exp, expSlime); EXPECT_TRUE(used > 0); - EXPECT_EQUAL(expSlime.get(), reply.root()); + ASSERT_TRUE(reply.hasSlime()); + EXPECT_EQUAL(expSlime, reply.slime()); } TEST("requireThatGetDocsumsExecute") { @@ -159,7 +160,7 @@ TEST("requireThatGetDocsumsExecute") { DocsumRequest::Source request(createRequest()); DocsumReply::UP reply = engine.getDocsums(std::move(request), client); EXPECT_TRUE(reply); - EXPECT_FALSE(reply->_root); + EXPECT_FALSE(reply->hasSlime()); } } diff --git a/searchcore/src/vespa/searchcore/proton/summaryengine/docsum_by_slime.cpp b/searchcore/src/vespa/searchcore/proton/summaryengine/docsum_by_slime.cpp index bd6a09c1387..3c8e5715a28 100644 --- a/searchcore/src/vespa/searchcore/proton/summaryengine/docsum_by_slime.cpp +++ b/searchcore/src/vespa/searchcore/proton/summaryengine/docsum_by_slime.cpp @@ -93,8 +93,8 @@ vespalib::Slime::UP DocsumBySlime::getDocsums(const Inspector & req) { DocsumReply::UP reply = _docsumServer.getDocsums(slimeToRequest(req)); - if (reply && reply->_root) { - return std::move(reply->_root); + if (reply && reply->hasSlime()) { + return reply->releaseSlime(); } else { LOG(warning, "got <null> docsum reply from back-end"); } diff --git a/searchcore/src/vespa/searchcore/proton/summaryengine/summaryengine.cpp b/searchcore/src/vespa/searchcore/proton/summaryengine/summaryengine.cpp index 91533011b40..0e01ee68cea 100644 --- a/searchcore/src/vespa/searchcore/proton/summaryengine/summaryengine.cpp +++ b/searchcore/src/vespa/searchcore/proton/summaryengine/summaryengine.cpp @@ -35,12 +35,8 @@ public: }; uint32_t getNumDocs(const DocsumReply &reply) { - if (reply._root) { - const Inspector &root = reply._root->get(); - return root[DOCSUMS].entries(); - } else { - return 0; - } + const Inspector &root = reply.root(); + return root[DOCSUMS].entries(); } VESPA_THREAD_STACK_TAG(summary_engine_executor) @@ -149,8 +145,8 @@ SummaryEngine::getDocsums(DocsumRequest::UP req) } updateDocsumMetrics(vespalib::to_s(req->getTimeUsed()), getNumDocs(*reply)); } - reply->request = std::move(req); - reply->my_issues = std::move(my_issues); + reply->setRequest(std::move(req)); + reply->setIssues(std::move(my_issues)); return reply; } diff --git a/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp b/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp index 973d2bcc15c..7eb7d1d0d80 100644 --- a/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp +++ b/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp @@ -478,14 +478,13 @@ TEST_F(DocsumRequestTest, require_that_global_ids_are_converted) { //----------------------------------------------------------------------------- struct DocsumReplyTest : ::testing::Test { - DocsumReply reply; + DocsumReply reply{std::make_unique<vespalib::Slime>()}; Converter::ProtoDocsumReply proto; void convert() { Converter::docsum_reply_to_proto(reply, proto); } }; TEST_F(DocsumReplyTest, require_that_slime_summaries_are_converted) { - reply._root = std::make_unique<Slime>(); - auto &list = reply._root->setArray(); + auto &list = reply.slime().setArray(); auto &doc0 = list.addObject(); doc0.setLong("my_field", 42); convert(); @@ -496,18 +495,18 @@ TEST_F(DocsumReplyTest, require_that_slime_summaries_are_converted) { } TEST_F(DocsumReplyTest, require_that_missing_root_slime_gives_empty_payload) { - reply._root.reset(); + reply.releaseSlime(); convert(); EXPECT_EQ(proto.slime_summaries().size(), 0); } TEST_F(DocsumReplyTest, require_that_issues_are_converted_to_errors) { - reply.my_issues = std::make_unique<UniqueIssues>(); - reply.my_issues->handle(vespalib::Issue("a")); - reply.my_issues->handle(vespalib::Issue("b")); - reply.my_issues->handle(vespalib::Issue("c")); - reply.my_issues->handle(vespalib::Issue("a")); - reply.my_issues->handle(vespalib::Issue("b")); + reply.setIssues(std::make_unique<UniqueIssues>()); + reply.issues().handle(vespalib::Issue("a")); + reply.issues().handle(vespalib::Issue("b")); + reply.issues().handle(vespalib::Issue("c")); + reply.issues().handle(vespalib::Issue("a")); + reply.issues().handle(vespalib::Issue("b")); convert(); ASSERT_EQ(proto.errors_size(), 3); EXPECT_EQ(proto.errors(0).message(), "a"); diff --git a/searchlib/src/tests/engine/proto_rpc_adapter/proto_rpc_adapter_test.cpp b/searchlib/src/tests/engine/proto_rpc_adapter/proto_rpc_adapter_test.cpp index 41f471f0a7a..7aa72b50ba4 100644 --- a/searchlib/src/tests/engine/proto_rpc_adapter/proto_rpc_adapter_test.cpp +++ b/searchlib/src/tests/engine/proto_rpc_adapter/proto_rpc_adapter_test.cpp @@ -49,12 +49,11 @@ struct MyDocsumServer : DocsumServer { DocsumReply::UP getDocsums(DocsumRequest::Source src, DocsumClient &client) override { auto req = src.release(); assert(req); - auto reply = std::make_unique<DocsumReply>(); - reply->_root = std::make_unique<Slime>(); - auto &list = reply->_root->setArray(); + auto reply = std::make_unique<DocsumReply>(std::make_unique<Slime>()); + auto &list = reply->slime().setArray(); list.addObject().setBool("use_root_slime", true); list.addObject().setString("ranking", req->ranking); - reply->request = std::move(req); + reply->setRequest(std::move(req)); std::this_thread::sleep_for(std::chrono::milliseconds(5)); client.getDocsumsDone(std::move(reply)); // simplified async response return std::unique_ptr<DocsumReply>(); diff --git a/searchlib/src/vespa/searchlib/engine/docsumreply.cpp b/searchlib/src/vespa/searchlib/engine/docsumreply.cpp index d546e087bbf..e8baad19a47 100644 --- a/searchlib/src/vespa/searchlib/engine/docsumreply.cpp +++ b/searchlib/src/vespa/searchlib/engine/docsumreply.cpp @@ -6,18 +6,19 @@ namespace search::engine { -DocsumReply::DocsumReply() : DocsumReply(vespalib::Slime::UP(nullptr)) { } +DocsumReply::DocsumReply() { } -DocsumReply::DocsumReply(vespalib::Slime::UP root) - : _root(std::move(root)) -{ } - -DocsumReply::~DocsumReply() { } +DocsumReply::DocsumReply(vespalib::Slime::UP root) : _slime(std::move(root)) { } vespalib::slime::Inspector & DocsumReply::root() const { - assert(_root); - return _root->get(); + return _slime ? _slime->get() : *vespalib::slime::NixValue::invalid(); +} + +std::unique_ptr<vespalib::Slime> DocsumReply::releaseSlime() { + return std::move(_slime); } +DocsumReply::~DocsumReply() { } + } diff --git a/searchlib/src/vespa/searchlib/engine/docsumreply.h b/searchlib/src/vespa/searchlib/engine/docsumreply.h index 10acd94a32a..6098dfe197f 100644 --- a/searchlib/src/vespa/searchlib/engine/docsumreply.h +++ b/searchlib/src/vespa/searchlib/engine/docsumreply.h @@ -8,23 +8,42 @@ #include <vespa/searchlib/common/unique_issues.h> #include <memory> #include <vespa/searchlib/engine/docsumrequest.h> +#include <cassert> namespace vespalib { class Slime; } namespace vespalib::slime { struct Inspector; } namespace search::engine { -struct DocsumReply -{ +class DocsumReply { +private: + DocsumRequest::UP _request; + std::unique_ptr<vespalib::Slime> _slime; + UniqueIssues::UP _issues; +public: using UP = std::unique_ptr<DocsumReply>; - mutable DocsumRequest::UP request; - std::unique_ptr<vespalib::Slime> _root; - UniqueIssues::UP my_issues; + DocsumReply(); + DocsumReply(std::unique_ptr<vespalib::Slime> root); + + bool hasSlime() const { return _slime.get() != nullptr; } + bool hasRequest() const { return _request.get() != nullptr; } + bool hasIssues() const { return _issues.get() != nullptr; } + + vespalib::Slime & slime() const { assert(hasSlime()); return *_slime; } + DocsumRequest& request() const { assert(hasRequest()); return *_request; } + UniqueIssues & issues() const { assert(hasIssues()); return *_issues; } + + void setRequest(DocsumRequest::UP request) { + _request = std::move(request); + } + void setIssues(UniqueIssues::UP issues) { + _issues = std::move(issues); + } + + std::unique_ptr<vespalib::Slime> releaseSlime(); vespalib::slime::Inspector & root() const; - DocsumReply(); - DocsumReply(std::unique_ptr<vespalib::Slime> root); ~DocsumReply(); }; diff --git a/searchlib/src/vespa/searchlib/engine/proto_converter.cpp b/searchlib/src/vespa/searchlib/engine/proto_converter.cpp index 64039f69cd5..52b247ae19e 100644 --- a/searchlib/src/vespa/searchlib/engine/proto_converter.cpp +++ b/searchlib/src/vespa/searchlib/engine/proto_converter.cpp @@ -168,17 +168,17 @@ ProtoConverter::docsum_request_from_proto(const ProtoDocsumRequest &proto, Docsu void ProtoConverter::docsum_reply_to_proto(const DocsumReply &reply, ProtoDocsumReply &proto) { - if (reply._root) { + if (reply.hasSlime()) { vespalib::SmartBuffer buf(4_Ki); - vespalib::slime::BinaryFormat::encode(*reply._root, buf); + vespalib::slime::BinaryFormat::encode(reply.slime(), buf); proto.set_slime_summaries(buf.obtain().data, buf.obtain().size); } - if (reply.my_issues) { - reply.my_issues->for_each_message([&](const vespalib::string &err_msg) - { - auto *err_obj = proto.add_errors(); - err_obj->set_message(err_msg); - }); + if (reply.hasIssues()) { + reply.issues().for_each_message([&](const vespalib::string &err_msg) + { + auto *err_obj = proto.add_errors(); + err_obj->set_message(err_msg); + }); } } diff --git a/searchlib/src/vespa/searchlib/engine/proto_rpc_adapter.cpp b/searchlib/src/vespa/searchlib/engine/proto_rpc_adapter.cpp index 5a3bfe42515..b4a50b10373 100644 --- a/searchlib/src/vespa/searchlib/engine/proto_rpc_adapter.cpp +++ b/searchlib/src/vespa/searchlib/engine/proto_rpc_adapter.cpp @@ -160,8 +160,8 @@ struct GetDocsumsCompletionHandler : DocsumClient { ProtoConverter::docsum_reply_to_proto(*reply, msg); encode_message(msg, *req.GetReturn()); stats.reply_size = (*req.GetReturn())[2]._data._len; - if (reply->request) { - stats.latency = vespalib::to_s(reply->request->getTimeUsed()); + if (reply->hasRequest()) { + stats.latency = vespalib::to_s(reply->request().getTimeUsed()); metrics.update_docsum_metrics(stats); } req.Return(); |