Added copyrights and javadocs. Logic and naming in LB cleaned up

5 files changed, 65 insertions, 24 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/CloseableChannel.java b/container-search/src/main/java/com/yahoo/search/dispatch/CloseableChannel.java
index 9a3e7e71031..838afa0c7fc 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/CloseableChannel.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/CloseableChannel.java
@@ -1,3 +1,4 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.search.dispatch;
 
 import com.yahoo.fs4.BasicPacket;
@@ -11,6 +12,9 @@ import java.io.Closeable;
 import java.io.IOException;
 import java.util.Optional;
 
+/**
+ * @author ollivir
+ */
 public class CloseableChannel implements Closeable {
     private FS4Channel channel;
 
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/DispatchedChannel.java b/container-search/src/main/java/com/yahoo/search/dispatch/DispatchedChannel.java
index 85926845647..00c59fbc979 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/DispatchedChannel.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/DispatchedChannel.java
@@ -1,3 +1,4 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.search.dispatch;
 
 import com.yahoo.prelude.fastsearch.FS4ResourcePool;
@@ -6,6 +7,11 @@ import com.yahoo.search.dispatch.SearchCluster.Node;
 
 import java.util.Optional;
 
+/**
+ * An extension to CloseableChannel that encapsulates the release of a LoadBalancer group allocation.
+ *
+ * @author ollivir
+ */
 public class DispatchedChannel extends CloseableChannel {
     private final SearchCluster.Group group;
     private final LoadBalancer loadBalancer;
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 f3677fd8d8d..be7cfea2017 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
@@ -284,7 +284,7 @@ public class Dispatcher extends AbstractComponent {
     }
 
     public Optional<CloseableChannel> getDispatchedChannel(Query query) {
-        Optional<SearchCluster.Group> groupInCluster = loadBalancer.getGroupForQuery(query);
+        Optional<SearchCluster.Group> groupInCluster = loadBalancer.takeGroupForQuery(query);
 
         return groupInCluster.flatMap(group -> {
             if(group.nodes().size() == 1) {
diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java
index 652ee134a87..d8e12980472 100644
--- a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java
+++ b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java
@@ -1,3 +1,4 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.search.dispatch;
 
 import com.yahoo.search.Query;
@@ -10,6 +11,12 @@ import java.util.Optional;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+/**
+ * LoadBalancer determines which group of content nodes should be accessed next for each search query when the internal java dispatcher is
+ * used.
+ *
+ * @author ollivir
+ */
 public class LoadBalancer {
     // The implementation here is a simplistic least queries in flight + round-robin load balancer
     // TODO: consider the options in com.yahoo.vespa.model.content.TuningDispatch
@@ -35,7 +42,14 @@ public class LoadBalancer {
         Collections.shuffle(scoreboard);
     }
 
-    public Optional<Group> getGroupForQuery(Query query) {
+    /**
+     * Select and allocate the search cluster group which is to be used for the provided query. Callers <b>must</b> call
+     * {@link #releaseGroup(Group)} symmetrically for each taken allocation.
+     *
+     * @param query
+     * @return The node group to target, or <i>empty</i> if the internal dispatch logic cannot be used
+     */
+    public Optional<Group> takeGroupForQuery(Query query) {
         if (!isInternallyDispatchable) {
             return Optional.empty();
         }
@@ -43,6 +57,12 @@ public class LoadBalancer {
         return allocateNextGroup();
     }
 
+    /**
+     * Release an allocation given by {@link #takeGroupForQuery(Query)}. The release must be done exactly once for each allocation.
+     *
+     * @param group
+     *            previously allocated group
+     */
     public void releaseGroup(Group group) {
         synchronized (this) {
             for (GroupSchedule sched : scoreboard) {
@@ -61,18 +81,13 @@ public class LoadBalancer {
             int index = needle;
             for (int i = 0; i < scoreboard.size(); i++) {
                 GroupSchedule sched = scoreboard.get(index);
-                index++;
-                if (index >= scoreboard.size()) {
-                    index = 0;
-                }
-                if (sched.group.hasSufficientCoverage() && (bestSchedule == null || sched.compareTo(bestSchedule) < 0)) {
+                if (sched.isPreferredOver(bestSchedule)) {
                     bestSchedule = sched;
                 }
+                index = nextScoreboardIndex(index);
             }
-            needle++;
-            if (needle >= scoreboard.size()) {
-                needle = 0;
-            }
+            needle = nextScoreboardIndex(needle);
+
             Group ret = null;
             if (bestSchedule != null) {
                 bestSchedule.adjustScore(1);
@@ -85,7 +100,15 @@ public class LoadBalancer {
         }
     }
 
-    public static class GroupSchedule implements Comparable<GroupSchedule> {
+    private int nextScoreboardIndex(int current) {
+        int next = current + 1;
+        if (next >= scoreboard.size()) {
+            next %= scoreboard.size();
+        }
+        return next;
+    }
+
+    private static class GroupSchedule {
         private final Group group;
         private int score;
 
@@ -94,9 +117,14 @@ public class LoadBalancer {
             this.score = 0;
         }
 
-        @Override
-        public int compareTo(GroupSchedule that) {
-            return this.score - that.score;
+        public boolean isPreferredOver(GroupSchedule other) {
+            if (! group.hasSufficientCoverage()) {
+                return false;
+            }
+            if (other == null) {
+                return true;
+            }
+            return this.score < other.score;
         }
 
         public void adjustScore(int amount) {
diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
index 443ab52d8cc..2ba991310f5 100644
--- a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
+++ b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java
@@ -14,6 +14,9 @@ import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertThat;
 
+/**
+ * @author ollivir
+ */
 public class LoadBalancerTest {
     @Test
     public void requreThatLoadBalancerServesSingleNodeSetups() {
@@ -21,7 +24,7 @@ public class LoadBalancerTest {
         SearchCluster cluster = new SearchCluster(88.0, Arrays.asList(n1), null, 1, null);
         LoadBalancer lb = new LoadBalancer(cluster);
 
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         Group group = grp.orElseGet(() -> {
             throw new AssertionFailedError("Expected a SearchCluster.Group");
         });
@@ -35,7 +38,7 @@ public class LoadBalancerTest {
         SearchCluster cluster = new SearchCluster(88.0, Arrays.asList(n1, n2), null, 1, null);
         LoadBalancer lb = new LoadBalancer(cluster);
 
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         Group group = grp.orElseGet(() -> {
             throw new AssertionFailedError("Expected a SearchCluster.Group");
         });
@@ -49,7 +52,7 @@ public class LoadBalancerTest {
         SearchCluster cluster = new SearchCluster(88.0, Arrays.asList(n1, n2), null, 2, null);
         LoadBalancer lb = new LoadBalancer(cluster);
 
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         assertThat(grp.isPresent(), is(false));
     }
 
@@ -62,7 +65,7 @@ public class LoadBalancerTest {
         SearchCluster cluster = new SearchCluster(88.0, Arrays.asList(n1, n2, n3, n4), null, 2, null);
         LoadBalancer lb = new LoadBalancer(cluster);
 
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         assertThat(grp.isPresent(), is(false));
     }
 
@@ -74,14 +77,14 @@ public class LoadBalancerTest {
         LoadBalancer lb = new LoadBalancer(cluster);
 
         // get first group
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         Group group = grp.get();
         int id1 = group.id();
         // release allocation
         lb.releaseGroup(group);
 
         // get second group
-        grp = lb.getGroupForQuery(null);
+        grp = lb.takeGroupForQuery(null);
         group = grp.get();
         assertThat(group.id(), not(equalTo(id1)));
     }
@@ -94,12 +97,12 @@ public class LoadBalancerTest {
         LoadBalancer lb = new LoadBalancer(cluster);
 
         // get first group
-        Optional<Group> grp = lb.getGroupForQuery(null);
+        Optional<Group> grp = lb.takeGroupForQuery(null);
         Group group = grp.get();
         int id1 = group.id();
 
         // get second group
-        grp = lb.getGroupForQuery(null);
+        grp = lb.takeGroupForQuery(null);
         group = grp.get();
         int id2 = group.id();
         assertThat(id2, not(equalTo(id1)));
@@ -107,7 +110,7 @@ public class LoadBalancerTest {
         lb.releaseGroup(group);
 
         // get third group
-        grp = lb.getGroupForQuery(null);
+        grp = lb.takeGroupForQuery(null);
         group = grp.get();
         assertThat(group.id(), equalTo(id2));
     }