Merge pull request #7594 from vespa-engine/balder/correct-coverage-reporting-on-adaptive-timeout

Balder/correct coverage reporting on adaptive timeout

11 files changed, 200 insertions, 27 deletions

diff --git a/searchcore/CMakeLists.txt b/searchcore/CMakeLists.txt
index 9d996d96dc7..d5d5544b88c 100644
--- a/searchcore/CMakeLists.txt
+++ b/searchcore/CMakeLists.txt
@@ -58,7 +58,7 @@ vespa_define_module(
     TESTS
     src/tests/applyattrupdates
     src/tests/fdispatch/randomrow
-    src/tests/fdispatch/search_path
+    src/tests/fdispatch/fnet_search
     src/tests/grouping
     src/tests/proton/attribute
     src/tests/proton/attribute/attribute_aspect_delayer
diff --git a/searchcore/src/testlist.txt b/searchcore/src/testlist.txt
index b74cd851052..f11d8d5c329 100644
--- a/searchcore/src/testlist.txt
+++ b/searchcore/src/testlist.txt
@@ -1,7 +1,7 @@
 ?tests/proton/proton
 tests/applyattrupdates
 tests/fdispatch/randomrow
-tests/fdispatch/search_path
+tests/fdispatch/fnet_search
 tests/grouping
 tests/proton/attribute
 tests/proton/attribute/attribute_manager
diff --git a/searchcore/src/tests/fdispatch/fnet_search/.gitignore b/searchcore/src/tests/fdispatch/fnet_search/.gitignore
new file mode 100644
index 00000000000..b525d6fcd38
--- /dev/null
+++ b/searchcore/src/tests/fdispatch/fnet_search/.gitignore
@@ -0,0 +1,2 @@
+searchcore_search_path_test_app
+searchcore_search_coverage_test_app
diff --git a/searchcore/src/tests/fdispatch/search_path/CMakeLists.txt b/searchcore/src/tests/fdispatch/fnet_search/CMakeLists.txt
index a3abaa868ff..c4e1608d6da 100644
--- a/searchcore/src/tests/fdispatch/search_path/CMakeLists.txt
+++ b/searchcore/src/tests/fdispatch/fnet_search/CMakeLists.txt
@@ -6,3 +6,13 @@ vespa_add_executable(searchcore_search_path_test_app TEST
     searchcore_fdispatch_search
 )
 vespa_add_test(NAME searchcore_search_path_test_app COMMAND searchcore_search_path_test_app)
+
+vespa_add_executable(searchcore_search_coverage_test_app TEST
+    SOURCES
+    search_coverage_test.cpp
+    DEPENDS
+    searchcore_fdispatch_search
+    searchcore_fdcommon
+    searchcore_grouping
+)
+vespa_add_test(NAME searchcore_search_coverage_test_app COMMAND searchcore_search_coverage_test_app)
diff --git a/searchcore/src/tests/fdispatch/search_path/DESC b/searchcore/src/tests/fdispatch/fnet_search/DESC
index 4bc24883896..4bc24883896 100644
--- a/searchcore/src/tests/fdispatch/search_path/DESC
+++ b/searchcore/src/tests/fdispatch/fnet_search/DESC
diff --git a/searchcore/src/tests/fdispatch/search_path/FILES b/searchcore/src/tests/fdispatch/fnet_search/FILES
index a38e13c26fd..a38e13c26fd 100644
--- a/searchcore/src/tests/fdispatch/search_path/FILES
+++ b/searchcore/src/tests/fdispatch/fnet_search/FILES
diff --git a/searchcore/src/tests/fdispatch/fnet_search/search_coverage_test.cpp b/searchcore/src/tests/fdispatch/fnet_search/search_coverage_test.cpp
new file mode 100644
index 00000000000..d598583437d
--- /dev/null
+++ b/searchcore/src/tests/fdispatch/fnet_search/search_coverage_test.cpp
@@ -0,0 +1,141 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+#include <vespa/vespalib/testkit/testapp.h>
+
+#include <vespa/searchcore/fdispatch/search/fnet_search.h>
+#include <vespa/searchlib/engine/searchreply.h>
+
+#include <vespa/log/log.h>
+LOG_SETUP("search_coverage_test");
+
+using namespace fdispatch;
+using search::engine::SearchReply;
+
+std::vector<FastS_FNET_SearchNode>
+createNodes(uint32_t count) {
+    std::vector<FastS_FNET_SearchNode> nodes;
+    nodes.reserve(count);
+    for (uint32_t partid(0); partid < count; partid++) {
+        nodes.emplace_back(nullptr, partid);
+    }
+    return nodes;
+}
+
+void
+query(FastS_FNET_SearchNode & node) {
+    node.DirtySetChannelOnlyForTesting((FNET_Channel *) 1);
+}
+
+void
+respond(FastS_FNET_SearchNode & node, size_t covered, size_t active, size_t soonActive, uint32_t degradeReason) {
+    node._qresult = new FS4Packet_QUERYRESULTX();
+    node._qresult->_coverageDocs = covered;
+    node._qresult->_activeDocs = active;
+    node._qresult->_soonActiveDocs = soonActive;
+    node._qresult->_coverageDegradeReason = degradeReason;
+}
+
+void
+respond(FastS_FNET_SearchNode & node, size_t covered, size_t active, size_t soonActive) {
+    respond(node, covered, active, soonActive, 0);
+}
+
+void disconnectNodes(std::vector<FastS_FNET_SearchNode> & nodes) {
+    for (auto & node : nodes) {
+        node.DirtySetChannelOnlyForTesting(nullptr);
+    }
+}
+TEST("testCoverageWhenAllNodesAreUp") {
+    std::vector<FastS_FNET_SearchNode> nodes = createNodes(4);
+    for (auto & node : nodes) {
+        query(node);
+        respond(node, 25, 30, 50);
+    }
+    FastS_SearchInfo si = FastS_FNET_Search::computeCoverage(nodes, 1, false);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(4u, si._nodesReplied);
+    EXPECT_EQUAL(100u, si._coverageDocs);
+    EXPECT_EQUAL(120u, si._activeDocs);
+    EXPECT_EQUAL(200u, si._soonActiveDocs);
+    EXPECT_EQUAL(0u, si._degradeReason);
+    disconnectNodes(nodes);
+}
+
+TEST("testCoverageWhenNoNodesAreUp") {
+    std::vector<FastS_FNET_SearchNode> nodes = createNodes(4);
+    for (auto & node : nodes) {
+        query(node);
+    }
+    FastS_SearchInfo si = FastS_FNET_Search::computeCoverage(nodes, 1, false);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(0u, si._nodesReplied);
+    EXPECT_EQUAL(0u, si._coverageDocs);
+    EXPECT_EQUAL(0u, si._activeDocs);
+    EXPECT_EQUAL(0u, si._soonActiveDocs);
+    EXPECT_EQUAL(SearchReply::Coverage::TIMEOUT, si._degradeReason);
+    disconnectNodes(nodes);
+}
+
+TEST("testCoverageWhenNoNodesAreUpWithAdaptiveTimeout") {
+    std::vector<FastS_FNET_SearchNode> nodes = createNodes(4);
+    for (auto & node : nodes) {
+        query(node);
+    }
+    FastS_SearchInfo si = FastS_FNET_Search::computeCoverage(nodes, 1, true);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(0u, si._nodesReplied);
+    EXPECT_EQUAL(0u, si._coverageDocs);
+    EXPECT_EQUAL(0u, si._activeDocs);
+    EXPECT_EQUAL(0u, si._soonActiveDocs);
+    EXPECT_EQUAL(SearchReply::Coverage::ADAPTIVE_TIMEOUT, si._degradeReason);
+    disconnectNodes(nodes);
+}
+
+TEST("testCoverageWhen1NodesIsDown") {
+    std::vector<FastS_FNET_SearchNode> nodes = createNodes(4);
+    for (auto & node : nodes) {
+        query(node);
+    }
+    respond(nodes[0], 25, 30, 50);
+    respond(nodes[2], 25, 30, 50);
+    respond(nodes[3], 25, 30, 50);
+
+    FastS_SearchInfo si = FastS_FNET_Search::computeCoverage(nodes, 1, false);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(3u, si._nodesReplied);
+    EXPECT_EQUAL(75u, si._coverageDocs);
+    EXPECT_EQUAL(120u, si._activeDocs);
+    EXPECT_EQUAL(200u, si._soonActiveDocs);
+    EXPECT_EQUAL(SearchReply::Coverage::TIMEOUT, si._degradeReason);
+
+    // Do not trigger dirty magic when you still have enough coverage in theory
+    si = FastS_FNET_Search::computeCoverage(nodes, 2, false);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(3u, si._nodesReplied);
+    EXPECT_EQUAL(75u, si._coverageDocs);
+    EXPECT_EQUAL(90u, si._activeDocs);
+    EXPECT_EQUAL(150u, si._soonActiveDocs);
+    EXPECT_EQUAL(0u, si._degradeReason);
+    disconnectNodes(nodes);
+}
+
+TEST("testCoverageWhen1NodeDoesnotReplyWithAdaptiveTimeout") {
+    std::vector<FastS_FNET_SearchNode> nodes = createNodes(4);
+    for (auto & node : nodes) {
+        query(node);
+    }
+    respond(nodes[0], 25, 30, 50);
+    respond(nodes[2], 25, 30, 50);
+    respond(nodes[3], 25, 30, 50);
+
+    FastS_SearchInfo si = FastS_FNET_Search::computeCoverage(nodes, 1, true);
+    EXPECT_EQUAL(4u, si._nodesQueried);
+    EXPECT_EQUAL(3u, si._nodesReplied);
+    EXPECT_EQUAL(75u, si._coverageDocs);
+    EXPECT_EQUAL(120u, si._activeDocs);
+    EXPECT_EQUAL(200u, si._soonActiveDocs);
+    EXPECT_EQUAL(SearchReply::Coverage::ADAPTIVE_TIMEOUT, si._degradeReason);
+    disconnectNodes(nodes);
+}
+
+
+TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchcore/src/tests/fdispatch/search_path/search_path_test.cpp b/searchcore/src/tests/fdispatch/fnet_search/search_path_test.cpp
index b62fb8d14f2..b62fb8d14f2 100644
--- a/searchcore/src/tests/fdispatch/search_path/search_path_test.cpp
+++ b/searchcore/src/tests/fdispatch/fnet_search/search_path_test.cpp
diff --git a/searchcore/src/tests/fdispatch/search_path/.gitignore b/searchcore/src/tests/fdispatch/search_path/.gitignore
deleted file mode 100644
index 7452ecf3ecc..00000000000
--- a/searchcore/src/tests/fdispatch/search_path/.gitignore
+++ /dev/null
@@ -1 +0,0 @@
-searchcore_search_path_test_app
diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp
index 73b92eaa2b7..c85e432140e 100644
--- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp
+++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.cpp
@@ -861,45 +861,60 @@ FastS_FNET_Search::MergeHits()
     }
 }
 
-void
-FastS_FNET_Search::CheckCoverage()
+FastS_SearchInfo
+FastS_FNET_Search::computeCoverage(const std::vector<FastS_FNET_SearchNode> & nodes,
+                                   uint32_t numSearchableCopies, bool adaptiveTimeout)
 {
-    uint64_t covDocs  = 0;
-    uint64_t activeDocs  = 0;
-    uint64_t soonActiveDocs = 0;
-    uint32_t degradedReason = 0;
-    uint16_t nodesQueried = 0;
-    uint16_t nodesReplied = 0;
+    FastS_SearchInfo si;
     size_t cntNone(0);
     size_t askedButNotAnswered(0);
 
-    for (const FastS_FNET_SearchNode & node : _nodes) {
+    for (const FastS_FNET_SearchNode & node : nodes) {
         if (node._qresult != nullptr) {
-            covDocs  += node._qresult->_coverageDocs;
-            activeDocs  += node._qresult->_activeDocs;
-            soonActiveDocs += node._qresult->_soonActiveDocs;
-            degradedReason |= node._qresult->_coverageDegradeReason;
-            nodesQueried += node._qresult->getNodesQueried();
-            nodesReplied += node._qresult->getNodesReplied();
+            si._coverageDocs  += node._qresult->_coverageDocs;
+            si._activeDocs  += node._qresult->_activeDocs;
+            si._soonActiveDocs += node._qresult->_soonActiveDocs;
+            si._degradeReason |= node._qresult->_coverageDegradeReason;
+            si._nodesQueried += node._qresult->getNodesQueried();
+            si._nodesReplied += node._qresult->getNodesReplied();
         } else {
-            nodesQueried++;
+            si._nodesQueried++;
             cntNone++;
             if (node.IsConnected()) {
                 askedButNotAnswered++;
             }
         }
     }
-    bool missingReplies = (askedButNotAnswered != 0) || (nodesQueried != nodesReplied);
-    const ssize_t missingParts = cntNone - (_dataset->getSearchableCopies() - 1);
-    if (((missingParts > 0) || (missingReplies && useAdaptiveTimeout())) && (cntNone != _nodes.size())) {
+    bool missingReplies = (askedButNotAnswered != 0) || (si._nodesQueried != si._nodesReplied);
+    const ssize_t missingParts = cntNone - (numSearchableCopies - 1);
+
+    if (missingReplies && adaptiveTimeout) {
+        // TODO This will not be correct when using multilevel dispatch and has timeout on anything, but leaf level.
+        //      We can live with that as leaf level failures are the likely ones.
+        if (si._nodesReplied ) {
+            si._activeDocs += askedButNotAnswered * si._activeDocs/si._nodesReplied;
+            si._soonActiveDocs += askedButNotAnswered * si._soonActiveDocs/si._nodesReplied;
+        }
+        si._degradeReason |= search::engine::SearchReply::Coverage::ADAPTIVE_TIMEOUT;
+    } else if (missingParts > 0) {
         // TODO This is a dirty way of anticipating missing coverage.
         // It should be done differently
-        activeDocs += missingParts * activeDocs/(_nodes.size() - cntNone);
-    }
-    if (missingReplies && useAdaptiveTimeout()) {
-        degradedReason |= search::engine::SearchReply::Coverage::ADAPTIVE_TIMEOUT;
+        if ((cntNone != nodes.size())) {
+            si._activeDocs += missingParts * si._activeDocs/(nodes.size() - cntNone);
+            si._soonActiveDocs += missingParts * si._soonActiveDocs/(nodes.size() - cntNone);
+        }
+        si._degradeReason |= search::engine::SearchReply::Coverage::TIMEOUT;
+
     }
-    _util.SetCoverage(covDocs, activeDocs, soonActiveDocs, degradedReason, nodesQueried, nodesReplied);
+    return si;
+}
+
+void
+FastS_FNET_Search::CheckCoverage()
+{
+    FastS_SearchInfo si = computeCoverage(_nodes, _dataset->getSearchableCopies(), useAdaptiveTimeout());
+    _util.SetCoverage(si._coverageDocs, si._activeDocs, si._soonActiveDocs,
+                      si._degradeReason, si._nodesQueried, si._nodesReplied);
 }
 
 
diff --git a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h
index 81e84e92bbd..a77b984ca28 100644
--- a/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h
+++ b/searchcore/src/vespa/searchcore/fdispatch/search/fnet_search.h
@@ -121,6 +121,10 @@ public:
         }
     }
 
+    void DirtySetChannelOnlyForTesting(FNET_Channel * channel) {
+        _channel = channel;
+    }
+
 
     void Disconnect()
     {
@@ -284,6 +288,8 @@ public:
     bool ShouldLimitHitsPerNode() const;
     void MergeHits();
     void CheckCoverage();
+    static FastS_SearchInfo computeCoverage(const std::vector<FastS_FNET_SearchNode> & nodes,
+                                            uint32_t numSearchableCopies, bool adaptiveTimeout);
     void CheckQueryTimes();
     void CheckDocsumTimes();
     void CheckQueryTimeout();