Dedupe absolutely identical federation targets

6 files changed, 88 insertions, 27 deletions

diff --git a/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java b/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
index 2c2264deec9..4f7c4749467 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/FederationSearcher.java
@@ -63,7 +63,9 @@ public class FederationSearcher extends ForkingSearcher {
 
     public static final String FEDERATION = "Federation";
 
+    /** A target for federation, containing a chain to which a federation query can be forwarded. */
     private static abstract class TargetHandler {
+
         abstract Chain<Searcher> getChain();
         abstract void modifyTargetQuery(Query query);
         abstract void modifyTargetResult(Result result);
@@ -75,13 +77,16 @@ public class FederationSearcher extends ForkingSearcher {
         public abstract FederationOptions federationOptions();
 
         @Override
-        public String toString() {
-            return getChain().getId().stringValue();
-        }
+        public String toString() { return getChain().getId().stringValue(); }
 
     }
 
+    /** 
+     * A handler representing a target created by the federation logic. 
+     * This is a value object, to ensure that identical target invocations are not invoked multiple times.
+     */
     private static class StandardTargetHandler extends TargetHandler {
+
         private final SearchChainInvocationSpec target;
         private final Chain<Searcher> chain;
 
@@ -91,9 +96,7 @@ public class FederationSearcher extends ForkingSearcher {
         }
 
         @Override
-        Chain<Searcher> getChain() {
-            return chain;
-        }
+        Chain<Searcher> getChain() { return chain; }
 
         @Override
         void modifyTargetQuery(Query query) {}
@@ -101,13 +104,27 @@ public class FederationSearcher extends ForkingSearcher {
         void modifyTargetResult(Result result) {}
 
         @Override
-        public FederationOptions federationOptions() {
-            return target.federationOptions;
+        public FederationOptions federationOptions() { return target.federationOptions; }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o == this) return true;
+            if ( ! ( o instanceof StandardTargetHandler)) return false;
+
+            StandardTargetHandler other = (StandardTargetHandler)o;
+            if ( ! other.chain.getId().equals(this.chain.getId())) return false;
+            if ( ! other.target.equals(this.target)) return false;
+            return true;
         }
-    }
+        
+        @Override
+        public int hashCode() { return chain.getId().hashCode() + 11 * target.hashCode(); }
 
+    }
 
+    /** A target handler where the target generation logic is delegated to the application provided target selector */
     private static class CustomTargetHandler<T> extends TargetHandler {
+
         private final TargetSelector<T> selector;
         private final FederationTarget<T> target;
 
@@ -135,18 +152,21 @@ public class FederationSearcher extends ForkingSearcher {
         public FederationOptions federationOptions() {
             return target.getFederationOptions();
         }
+
     }
 
     private static class ExecutionInfo {
-        final TargetHandler targetHandler;
-        final FederationOptions federationOptions;
-        final FutureResult futureResult;
+
+        private final TargetHandler targetHandler;
+        private final FederationOptions federationOptions;
+        private final FutureResult futureResult;
 
         public ExecutionInfo(TargetHandler targetHandler, FederationOptions federationOptions, FutureResult futureResult) {
             this.targetHandler = targetHandler;
             this.federationOptions = federationOptions;
             this.futureResult = futureResult;
         }
+
     }
 
     private static class CompoundKey {
@@ -304,7 +324,7 @@ public class FederationSearcher extends ForkingSearcher {
             throw new RuntimeException("Invalid federation config, " + target.id() + " != " + searchChain.searchChainId());
 
         builder.addSearchChain(ComponentId.fromString(searchChain.searchChainId()),
-                federationOptions(searchChain), searchChain.documentTypes());
+                               federationOptions(searchChain), searchChain.documentTypes());
     }
 
     private static void addSourceForProvider(SearchChainResolver.Builder builder, FederationConfig.Target target,
@@ -325,13 +345,13 @@ public class FederationSearcher extends ForkingSearcher {
                 setRequestTimeoutInMilliseconds(searchChain.requestTimeoutMillis());
     }
 
-    private static long calculateTimeout(Query query, List<TargetHandler> targets) {
+    private static long calculateTimeout(Query query, Collection<TargetHandler> targets) {
 
         class PartitionByOptional {
             final List<TargetHandler> mandatoryTargets;
             final List<TargetHandler> optionalTargets;
 
-            PartitionByOptional(List<TargetHandler> targets) {
+            PartitionByOptional(Collection<TargetHandler> targets) {
                 List<TargetHandler> mandatoryTargets = new ArrayList<>();
                 List<TargetHandler> optionalTargets = new ArrayList<>();
 
@@ -864,10 +884,10 @@ public class FederationSearcher extends ForkingSearcher {
         Results<TargetHandler, ErrorMessage> regularTargetHandlers = resolveSearchChains(prunedTargets, execution.searchChainRegistry());
         query.errors().addAll(regularTargetHandlers.errors());
 
-        List<TargetHandler> targetHandlers = new ArrayList<>(regularTargetHandlers.data());
+        Set<TargetHandler> targetHandlers = new LinkedHashSet<>(regularTargetHandlers.data());
         targetHandlers.addAll(getAdditionalTargets(query, execution, targetSelector));
 
-        final long targetsTimeout = calculateTimeout(query, targetHandlers);
+        long targetsTimeout = calculateTimeout(query, targetHandlers);
         if (targetsTimeout < 0)
             return new Result(query, ErrorMessage.createTimeout("Timed out when about to federate"));
 
@@ -875,7 +895,8 @@ public class FederationSearcher extends ForkingSearcher {
 
         if (targetHandlers.size() == 0) {
             return mergedResults;
-        } else if (targetHandlers.size() == 1 &&  ! shouldExecuteTargetLongerThanThread(query, targetHandlers.get(0))) {
+        } else if (targetHandlers.size() == 1 &&  
+                   ! shouldExecuteTargetLongerThanThread(query, targetHandlers.iterator().next())) {
             TargetHandler chain = first(targetHandlers);
             searchSingleTarget(query, mergedResults, chain, targetsTimeout, execution);
         } else {
@@ -885,8 +906,8 @@ public class FederationSearcher extends ForkingSearcher {
         return mergedResults;
     }
 
-    private void traceTargets(Query query, List<TargetHandler> targetHandlers) {
-        final int traceFederationLevel = 2;
+    private void traceTargets(Query query, Collection<TargetHandler> targetHandlers) {
+        int traceFederationLevel = 2;
         if ( ! query.isTraceable(traceFederationLevel)) return;
         query.trace("Federating to " + targetHandlers, traceFederationLevel);
     }
@@ -909,7 +930,7 @@ public class FederationSearcher extends ForkingSearcher {
             Chain<Searcher> chain = registry.getChain(target.searchChainId);
             if (chain == null) {
                 targetHandlers.addError(ErrorMessage.createIllegalQuery(
-                        "Could not find search chain '" + target.searchChainId + "'"));
+                                        "Could not find search chain '" + target.searchChainId + "'"));
             } else {
                 targetHandlers.addData(new StandardTargetHandler(target, chain));
             }
diff --git a/container-search/src/main/java/com/yahoo/search/federation/selection/FederationTarget.java b/container-search/src/main/java/com/yahoo/search/federation/selection/FederationTarget.java
index 676292d6a3a..bcc82e51bb8 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/selection/FederationTarget.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/selection/FederationTarget.java
@@ -16,6 +16,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
  * @author tonytv
  */
 public final class FederationTarget<T> {
+
     private final Chain<Searcher> chain;
     private final FederationOptions federationOptions;
     private final T customData;
@@ -65,4 +66,5 @@ public final class FederationTarget<T> {
         result = 31 * result + federationOptions.hashCode();
         return result;
     }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/federation/selection/TargetSelector.java b/container-search/src/main/java/com/yahoo/search/federation/selection/TargetSelector.java
index 0f6bf2d5b71..72de1516e74 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/selection/TargetSelector.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/selection/TargetSelector.java
@@ -21,6 +21,7 @@ import java.util.Collection;
  * @author tonytv
  */
 public interface TargetSelector<T> {
+
     Collection<FederationTarget<T>> getTargets(Query query, ChainRegistry<Searcher> searcherChainRegistry);
 
     /**
@@ -32,4 +33,5 @@ public interface TargetSelector<T> {
      * For modifying the result produced by the target.
      */
     void modifyTargetResult(FederationTarget<T> target, Result result);
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainInvocationSpec.java b/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainInvocationSpec.java
index 7e82801d85f..c4d168af085 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainInvocationSpec.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainInvocationSpec.java
@@ -1,24 +1,30 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.search.federation.sourceref;
 
+import com.google.common.collect.ImmutableList;
 import com.yahoo.component.ComponentId;
 import com.yahoo.search.searchchain.model.federation.FederationOptions;
 
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Specifices which search chain should be run and how it should be run.
+ * This is a value object.
  *
  * @author tonytv
  */
 public class SearchChainInvocationSpec implements Cloneable {
+
     public final ComponentId searchChainId;
 
+    /** The source to invoke, or null if none */
     public final ComponentId source;
+    /** The provider to invoke, or null if none */
     public final ComponentId provider;
 
     public final FederationOptions federationOptions;
-    public final List<String> documentTypes;
+    public final ImmutableList<String> documentTypes;
 
     SearchChainInvocationSpec(ComponentId searchChainId,
                               ComponentId source, ComponentId provider, FederationOptions federationOptions,
@@ -27,11 +33,35 @@ public class SearchChainInvocationSpec implements Cloneable {
         this.source = source;
         this.provider = provider;
         this.federationOptions = federationOptions;
-        this.documentTypes = documentTypes;
+        this.documentTypes = ImmutableList.copyOf(documentTypes);
     }
 
     @Override
     public SearchChainInvocationSpec clone() throws CloneNotSupportedException {
         return (SearchChainInvocationSpec)super.clone();
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (o == this) return true;
+        if ( ! ( o instanceof SearchChainInvocationSpec)) return false;
+
+        SearchChainInvocationSpec other = (SearchChainInvocationSpec)o;
+        if ( ! Objects.equals(this.searchChainId, other.searchChainId)) return false;
+        if ( ! Objects.equals(this.source, other.source)) return false;
+        if ( ! Objects.equals(this.provider, other.provider)) return false;
+        if ( ! Objects.equals(this.federationOptions, other.federationOptions)) return false;
+        if ( ! Objects.equals(this.documentTypes, other.documentTypes)) return false;
+        return true;
+    }
+
+    @Override
+    public int hashCode() { 
+        return searchChainId.hashCode() + 
+               (source != null ? 3 * source.hashCode() : 0) +
+               (provider != null ? 5 * provider.hashCode(): 0) +
+               federationOptions.hashCode() +
+               documentTypes.hashCode();
+    }
+
 }
diff --git a/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainResolver.java b/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainResolver.java
index fc70fb5e5e7..bdd40ca83f4 100644
--- a/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainResolver.java
+++ b/container-search/src/main/java/com/yahoo/search/federation/sourceref/SearchChainResolver.java
@@ -40,6 +40,7 @@ import java.util.TreeSet;
  * @author tonytv
  */
 public class SearchChainResolver {
+
     private final ComponentRegistry<Target> targets;
     private final SortedSet<Target> defaultTargets;
 
diff --git a/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java b/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
index ec6bf9661c6..bf22a309058 100644
--- a/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
+++ b/container-search/src/main/java/com/yahoo/search/searchchain/model/federation/FederationOptions.java
@@ -5,6 +5,7 @@ import net.jcip.annotations.Immutable;
 
 /**
  * Options for controlling federation to a single source.
+ * This is a value object.
  *
  * @author tonytv
  */
@@ -103,13 +104,17 @@ public class FederationOptions implements Cloneable {
 
     @Override
     public boolean equals(Object other) {
-        return (other instanceof FederationOptions) &&
-                equals((FederationOptions) other);
+        return (other instanceof FederationOptions) && equals((FederationOptions) other);
     }
-
+    
     public boolean equals(FederationOptions other) {
         return getOptional() == other.getOptional() &&
-                getTimeoutInMilliseconds() == other.getTimeoutInMilliseconds();
+               getTimeoutInMilliseconds() == other.getTimeoutInMilliseconds();
+    }
+    
+    @Override
+    public int hashCode() {
+        return ( getOptional() ? 1 : 0 ) + 13 * getTimeoutInMilliseconds();
     }
 
     @Override