Address review comments

Author: davidkyle

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/InferenceProcessor.java

@@ -11,7 +11,6 @@
 import org.elasticsearch.ingest.IngestDocument;
 import org.elasticsearch.ingest.Processor;
 
-import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.BiConsumer;
@@ -43,8 +42,7 @@ public void execute(IngestDocument ingestDocument, BiConsumer<IngestDocument, Ex
 
     @Override
     public IngestDocument execute(IngestDocument ingestDocument) {
-        assert false : "The async override of execute() must be used";
-        return null;
+        throw new UnsupportedOperationException("The async override execute(document, handler) must be used");
     }
 
     @Override
@@ -54,11 +52,11 @@ public String getType() {
 
     public static final class Factory implements Processor.Factory {
 
-        private Map<String, Model> loadedModels;
-        private Map<String, ModelLoader> modelLoaders;
+        private final Map<String, Model> loadedModels;
+        private final Map<String, ModelLoader> modelLoaders;
 
         public Factory(Map<String, ModelLoader> modelLoaders) {
-            loadedModels = new ConcurrentHashMap<>();
+            this.loadedModels = new ConcurrentHashMap<>();
             this.modelLoaders = modelLoaders;
         }
 

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/Model.java

@@ -20,7 +20,7 @@ public interface Model {
      * be called either with the updated (or new) document or an {@code Exception}.
      *
      * @param document  Document to infer on
-     * @param handler   The handler must be called
+     * @param handler   The handler that must be called
      */
     void infer(IngestDocument document, BiConsumer<IngestDocument, Exception> handler);
 }

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/ModelLoader.java

@@ -10,7 +10,11 @@
 
 public interface ModelLoader {
     /**
-     *  Load the model with the given {@code modelId} and {@code config}
+     *  Load the model with the given {@code modelId} and {@code config}.
+     *
+     *  Model specific config should can read from {@code config} and should
+     *  be removed from the map e.g with the use of {@link org.elasticsearch.ingest.ConfigurationUtils}
+     *
      *
      * @param modelId        The Id of the model to load
      * @param processorTag   The ingest processor tag
@@ -22,9 +26,12 @@ public interface ModelLoader {
 
     /**
      * Inference processors must remove their configuration from the {@code config} map.
-     * For the case when a model has already been loaded
+     * In the case when a model has already been loaded and {@link #load(String, String, boolean, Map)}
+     * will not be called this use this method to strip the model specific config from the map.
      *
-     * This method should be used when {@link #load(String, String, boolean, Map)} isn't.
+     * This method should only be used if {@link #load(String, String, boolean, Map)} isn't
+     * called on the same {@code config} as {@link #load(String, String, boolean, Map)} will
+     * also remove the processor's configuration from the map.
      *
      * @param processorTag  The ingest processor tag
      * @param config        The ingest pipeline config

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/sillymodel/SillyModel.java

@@ -20,7 +20,7 @@ public class SillyModel implements Model {
 
     private static final String TARGET_FIELD = "hotdog_or_not";
 
-    private Random random;
+    private final Random random;
 
     public SillyModel() {
         random = Randomness.get();

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/sillymodel/SillyModelLoader.java

@@ -58,7 +58,7 @@ public void consumeConfiguration(String processorTag, Map<String, Object> config
     }
 
 
-    public void load(String id, String index, ActionListener<Model> listener) {
+    private void load(String id, String index, ActionListener<Model> listener) {
         client.prepareGet(index, null, id).execute(ActionListener.wrap(
                 response -> {
                     if (response.isExists()) {

x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/inference/InferenceProcessorTests.java

@@ -23,16 +23,17 @@ public class InferenceProcessorTests extends ESTestCase {
 
     public void testFactory() throws Exception {
         String processorTag = randomAlphaOfLength(10);
+        String modelType = "test";
 
         Model mockModel = mock(Model.class);
         ModelLoader mockLoader = mock(ModelLoader.class);
         when(mockLoader.load(eq("k2"), eq(processorTag), eq(false), any())).thenReturn(mockModel);
 
-        InferenceProcessor.Factory factory = new InferenceProcessor.Factory(Map.of("test", mockLoader));
+        InferenceProcessor.Factory factory = new InferenceProcessor.Factory(Collections.singletonMap(modelType, mockLoader));
 
         Map<String, Object> config = new HashMap<>();
         config.put("model_id", "k2");
-        config.put("model_type", "test");
+        config.put("model_type", modelType);
 
         InferenceProcessor processor = factory.create(Collections.emptyMap(), processorTag, config);
         assertThat(processor.getTag(), equalTo(processorTag));