Merge pull request #19555 from vespa-engine/arnej/docsumreply-class

make DocsumReply a proper class

11 files changed, 119 insertions, 67 deletions

diff --git a/searchcore/src/tests/proton/docsummary/docsummary.cpp b/searchcore/src/tests/proton/docsummary/docsummary.cpp
index 916b88c19c4..ffe720059c6 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.hasResult());
+    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..d942abd28c2 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.hasResult());
+    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->hasResult());
     }
 }
 
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..a5ad7618c84 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->hasResult()) {
+        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..ec5c8c9b72d 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)
@@ -110,11 +106,8 @@ DocsumReply::UP
 SummaryEngine::getDocsums(DocsumRequest::Source request, DocsumClient & client)
 {
     if (_closed) {
-        LOG(warning, "Receiving docsumrequest after engine has been shutdown");
+        vespalib::Issue::report("Received docsum request after engine has been shutdown");
         auto ret = std::make_unique<DocsumReply>();
-
-        // TODO: Notify closed.
-
         return ret;
     }
     if (_async) {
@@ -131,8 +124,7 @@ SummaryEngine::getDocsums(DocsumRequest::UP req)
     auto my_issues = std::make_unique<search::UniqueIssues>();
     auto capture_issues = vespalib::Issue::listen(*my_issues);
 
-    DocsumReply::UP reply = std::make_unique<DocsumReply>();
-
+    DocsumReply::UP reply;
     if (req) {
         ISearchHandler::SP searchHandler = getSearchHandler(DocTypeName(*req));
         if (searchHandler) {
@@ -149,9 +141,11 @@ 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);
-
+    if (! reply) {
+        reply = std::make_unique<DocsumReply>();
+    }
+    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..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();