diff options
author | Arne H Juul <arnej27959@users.noreply.github.com> | 2021-10-15 13:54:19 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-10-15 13:54:19 +0200 |
commit | 0b1564f56523601b9f125a23c578801c46449fcb (patch) | |
tree | 0f27fd6ea4105c776e733358a00d9b9adc76a738 /searchlib | |
parent | 863755f751d4142dc555473ae82d6c9c8f141092 (diff) | |
parent | f482d59420cd8c2b858643c4c1a9668d9990774f (diff) |
Merge pull request #19555 from vespa-engine/arnej/docsumreply-class
make DocsumReply a proper class
Diffstat (limited to 'searchlib')
7 files changed, 94 insertions, 40 deletions
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..d0c356bf879 100644 --- a/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp +++ b/searchlib/src/tests/engine/proto_converter/proto_converter_test.cpp @@ -478,36 +478,41 @@ TEST_F(DocsumRequestTest, require_that_global_ids_are_converted) { //----------------------------------------------------------------------------- struct DocsumReplyTest : ::testing::Test { + Slime &slime; DocsumReply reply; Converter::ProtoDocsumReply proto; void convert() { Converter::docsum_reply_to_proto(reply, proto); } + DocsumReplyTest(std::unique_ptr<Slime> slime_in) + : slime(*slime_in), reply(std::move(slime_in)) + {} + DocsumReplyTest() : DocsumReplyTest(std::make_unique<Slime>()) {} }; TEST_F(DocsumReplyTest, require_that_slime_summaries_are_converted) { - reply._root = std::make_unique<Slime>(); - auto &list = reply._root->setArray(); + auto &list = slime.setArray(); auto &doc0 = list.addObject(); doc0.setLong("my_field", 42); convert(); const auto &mem = proto.slime_summaries(); - Slime slime; - EXPECT_EQ(BinaryFormat::decode(Memory(mem.data(), mem.size()), slime), mem.size()); - EXPECT_EQ(slime.get()[0]["my_field"].asLong(), 42); + Slime decoded; + EXPECT_EQ(BinaryFormat::decode(Memory(mem.data(), mem.size()), decoded), mem.size()); + EXPECT_EQ(decoded.get()[0]["my_field"].asLong(), 42); } 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")); + auto my_issues = std::make_unique<UniqueIssues>(); + my_issues->handle(vespalib::Issue("a")); + my_issues->handle(vespalib::Issue("b")); + my_issues->handle(vespalib::Issue("c")); + my_issues->handle(vespalib::Issue("a")); + my_issues->handle(vespalib::Issue("b")); + reply.setIssues(std::move(my_issues)); 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..33a40b76a51 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 slime = std::make_unique<Slime>(); + auto &list = slime->setArray(); list.addObject().setBool("use_root_slime", true); list.addObject().setString("ranking", req->ranking); - reply->request = std::move(req); + auto reply = std::make_unique<DocsumReply>(std::move(slime), 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/common/unique_issues.h b/searchlib/src/vespa/searchlib/common/unique_issues.h index 5b25820c408..79cb7a9aa23 100644 --- a/searchlib/src/vespa/searchlib/common/unique_issues.h +++ b/searchlib/src/vespa/searchlib/common/unique_issues.h @@ -23,6 +23,7 @@ public: fun(msg); } } + size_t size() const { return _messages.size(); } }; } diff --git a/searchlib/src/vespa/searchlib/engine/docsumreply.cpp b/searchlib/src/vespa/searchlib/engine/docsumreply.cpp index d546e087bbf..6a4e2e5fb5a 100644 --- a/searchlib/src/vespa/searchlib/engine/docsumreply.cpp +++ b/searchlib/src/vespa/searchlib/engine/docsumreply.cpp @@ -4,20 +4,41 @@ #include <vespa/vespalib/data/slime/slime.h> #include <cassert> +using vespalib::Slime; +using vespalib::slime::Inspector; +using vespalib::slime::NixValue; + namespace search::engine { -DocsumReply::DocsumReply() : DocsumReply(vespalib::Slime::UP(nullptr)) { } +DocsumReply::DocsumReply(std::unique_ptr<vespalib::Slime> root, + DocsumRequest::UP request, + UniqueIssues::UP issues) + : _slime(std::move(root)), + _request(std::move(request)), + _issues(std::move(issues)) +{} + +DocsumReply::DocsumReply(Slime::UP root, DocsumRequest::UP request) + : DocsumReply(std::move(root), std::move(request), {}) {} -DocsumReply::DocsumReply(vespalib::Slime::UP root) - : _root(std::move(root)) -{ } +DocsumReply::DocsumReply(Slime::UP root) + : DocsumReply(std::move(root), {}, {}) {} -DocsumReply::~DocsumReply() { } +DocsumReply::DocsumReply() = default; vespalib::slime::Inspector & DocsumReply::root() const { - assert(_root); - return _root->get(); + return _slime ? _slime->get() : *NixValue::invalid(); +} + +bool DocsumReply::hasResult() const { + return root().valid(); } +std::unique_ptr<Slime> DocsumReply::releaseSlime() { + return std::move(_slime); +} + +DocsumReply::~DocsumReply() = default; + } diff --git a/searchlib/src/vespa/searchlib/engine/docsumreply.h b/searchlib/src/vespa/searchlib/engine/docsumreply.h index 10acd94a32a..0caa848e4ef 100644 --- a/searchlib/src/vespa/searchlib/engine/docsumreply.h +++ b/searchlib/src/vespa/searchlib/engine/docsumreply.h @@ -8,23 +8,51 @@ #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: + std::unique_ptr<vespalib::Slime> _slime; + DocsumRequest::UP _request; + 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(std::unique_ptr<vespalib::Slime> root, + DocsumRequest::UP request, + UniqueIssues::UP issues); - vespalib::slime::Inspector & root() const; + DocsumReply(std::unique_ptr<vespalib::Slime> root, + DocsumRequest::UP request); - DocsumReply(); DocsumReply(std::unique_ptr<vespalib::Slime> root); + + DocsumReply(); + + bool hasResult() const; + bool hasRequest() const { return (_request.get() != nullptr); } + bool hasIssues() const { return _issues && (_issues->size() > 0); } + + const vespalib::Slime & slime() const { assert(_slime.get()); return *_slime; } + const DocsumRequest& request() const { assert(_request.get()); return *_request; } + const UniqueIssues & issues() const { assert(_issues.get()); 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(); }; diff --git a/searchlib/src/vespa/searchlib/engine/proto_converter.cpp b/searchlib/src/vespa/searchlib/engine/proto_converter.cpp index 64039f69cd5..8b949d9ed4e 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.hasResult()) { 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(); |