Use OptionalInt to signify missing data instead of -1

4 files changed, 27 insertions, 13 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java
index 612941ece7a..c6ce6b15efe 100644
--- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java
+++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java
@@ -24,6 +24,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.Set;
 
 /**
@@ -69,7 +70,7 @@ public class FS4InvokerFactory {
      * @return Optional containing the SearchInvoker or <i>empty</i> if some node in the
      *         list is invalid and the remaining coverage is not sufficient
      */
-    public Optional<SearchInvoker> getSearchInvoker(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage) {
+    public Optional<SearchInvoker> getSearchInvoker(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage) {
         List<SearchInvoker> invokers = new ArrayList<>(nodes.size());
         Set<Integer> failed = null;
         for (Node node : nodes) {
@@ -90,7 +91,7 @@ public class FS4InvokerFactory {
             }
         }
 
-        if (failed != null) {
+        if (failed != null && groupId.isPresent()) {
             List<Node> success = new ArrayList<>(nodes.size() - failed.size());
             for (Node node : nodes) {
                 if (!failed.contains(node.key())) {
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
index 1d39cffa9d2..146b132be22 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/Dispatcher.java
@@ -21,6 +21,7 @@ import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.Set;
 
 /**
@@ -106,7 +107,7 @@ public class Dispatcher extends AbstractComponent {
 
     @FunctionalInterface
     private interface SearchInvokerSupplier {
-        Optional<SearchInvoker> supply(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage);
+        Optional<SearchInvoker> supply(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage);
     }
 
     // build invoker based on searchpath
@@ -121,7 +122,7 @@ public class Dispatcher extends AbstractComponent {
                 return Optional.empty();
             } else {
                 query.trace(false, 2, "Dispatching internally with search path ", searchPath);
-                return invokerFactory.supply(query, -1, nodes, true);
+                return invokerFactory.supply(query, OptionalInt.empty(), nodes, true);
             }
         } catch (InvalidSearchPathException e) {
             return Optional.of(new SearchErrorInvoker(ErrorMessage.createIllegalQuery(e.getMessage())));
@@ -133,7 +134,7 @@ public class Dispatcher extends AbstractComponent {
         if (directNode.isPresent()) {
             Node node = directNode.get();
             query.trace(false, 2, "Dispatching directly to ", node);
-            return invokerFactory.supply(query, -1, Arrays.asList(node), true);
+            return invokerFactory.supply(query, OptionalInt.empty(), Arrays.asList(node), true);
         }
 
         int covered = searchCluster.groupsWithSufficientCoverage();
@@ -148,7 +149,8 @@ public class Dispatcher extends AbstractComponent {
             }
             Group group = groupInCluster.get();
             boolean acceptIncompleteCoverage = (i == max - 1);
-            Optional<SearchInvoker> invoker = invokerFactory.supply(query, group.id(), group.nodes(), acceptIncompleteCoverage);
+            Optional<SearchInvoker> invoker = invokerFactory.supply(query, OptionalInt.of(group.id()), group.nodes(),
+                    acceptIncompleteCoverage);
             if (invoker.isPresent()) {
                 query.trace(false, 2, "Dispatching internally to search group ", group.id());
                 query.getModel().setSearchPath("/" + group.id());
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
index e497ef6751b..473fe7a1a12 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/searchcluster/SearchCluster.java
@@ -18,6 +18,7 @@ import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.FutureTask;
@@ -331,9 +332,10 @@ public class SearchCluster implements NodeManager<Node> {
         }
     }
 
-    private void logIfInsufficientCoverage(boolean sufficient, int groupId, int nodes) {
+    private void logIfInsufficientCoverage(boolean sufficient, OptionalInt groupId, int nodes) {
         if (!sufficient) {
-            log.warning(() -> String.format("Coverage of group %s is only %d/%d (requires %d)", groupId, nodes, groupSize(),
+            String group = groupId.isPresent()? Integer.toString(groupId.getAsInt()) : "(unspecified)";
+            log.warning(() -> String.format("Coverage of group %s is only %d/%d (requires %d)", group, nodes, groupSize(),
                     groupSize() - dispatchConfig.maxNodesDownPerGroup()));
         }
     }
@@ -341,14 +343,22 @@ public class SearchCluster implements NodeManager<Node> {
     /**
      * Calculate whether a subset of nodes in a group has enough coverage
      */
-    public boolean isPartialGroupCoverageSufficient(int groupId, List<Node> nodes) {
+    public boolean isPartialGroupCoverageSufficient(OptionalInt knownGroupId, List<Node> nodes) {
         if (orderedGroups.size() == 1) {
             boolean sufficient = nodes.size() >= groupSize() - dispatchConfig.maxNodesDownPerGroup();
-            logIfInsufficientCoverage(sufficient, groupId, nodes.size());
+            logIfInsufficientCoverage(sufficient, knownGroupId, nodes.size());
             return sufficient;
         }
 
-        int nodesInGroup = groups.get(groupId).nodes().size();
+        if (knownGroupId.isEmpty()) {
+            return false;
+        }
+        int groupId = knownGroupId.getAsInt();
+        Group group = groups.get(groupId);
+        if (group == null) {
+            return false;
+        }
+        int nodesInGroup = group.nodes().size();
         long sumOfActiveDocuments = 0;
         int otherGroups = 0;
         for (Group g : orderedGroups) {
@@ -363,7 +373,7 @@ public class SearchCluster implements NodeManager<Node> {
         }
         long averageDocumentsInOtherGroups = sumOfActiveDocuments / otherGroups;
         boolean sufficient = isGroupCoverageSufficient(nodes.size(), nodesInGroup, activeDocuments, averageDocumentsInOtherGroups);
-        logIfInsufficientCoverage(sufficient, groupId, nodes.size());
+        logIfInsufficientCoverage(sufficient, knownGroupId, nodes.size());
         return sufficient;
     }
 }
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
index fae659b0d70..708caafa3f5 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/DispatcherTest.java
@@ -11,6 +11,7 @@ import org.junit.Test;
 
 import java.util.List;
 import java.util.Optional;
+import java.util.OptionalInt;
 
 import static com.yahoo.search.dispatch.MockSearchCluster.createDispatchConfig;
 import static org.hamcrest.Matchers.is;
@@ -112,7 +113,7 @@ public class DispatcherTest {
         }
 
         @Override
-        public Optional<SearchInvoker> getSearchInvoker(Query query, int groupId, List<Node> nodes, boolean acceptIncompleteCoverage) {
+        public Optional<SearchInvoker> getSearchInvoker(Query query, OptionalInt groupId, List<Node> nodes, boolean acceptIncompleteCoverage) {
             if (step >= events.length) {
                 throw new RuntimeException("Was not expecting more calls to getSearchInvoker");
             }