madrob commented on a change in pull request #1432:
URL: https://github.com/apache/lucene-solr/pull/1432#discussion_r412493389



##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {
+    try {
+      pluginInfos = 
ContainerPluginsApi.plugins(coreContainer.zkClientSupplier);
+    } catch (IOException e) {
+      log.error("Could not read plugins data", e);
+      return;
+    }
+    if(pluginInfos.isEmpty()) return;
+
+    for (Map.Entry<String, Object> e : pluginInfos.entrySet()) {
+      PluginMeta info = null;
+      try {
+        info = mapper.readValue(Utils.toJSON(e.getValue()), PluginMeta.class);
+      } catch (IOException ioException) {
+        log.error("Invalid apiInfo configuration :", ioException);
+      }
+
+      ApiInfo apiInfo = null;
+      try {
+        List<String> errs = new ArrayList<>();
+        apiInfo = new ApiInfo(info, errs);
+        if (!errs.isEmpty()) {
+          log.error(StrUtils.join(errs, ','));
+          continue;
+        }
+      } catch (Exception ex) {
+        log.error("unable to instantiate apiInfo ", ex);
+        continue;
+      }
+
+      String path = pluginNameVsPath.get(e.getKey());
+      if (path == null) {
+        // there is a new apiInfo . let's register it
+        try {
+          apiInfo.init();
+          ApiHolder holder = new ApiHolder(apiInfo);
+          plugins.put(holder.key, holder);
+          pluginNameVsPath.put(apiInfo.info.name, holder.key);
+          containerApiBag.register(holder, Collections.EMPTY_MAP);

Review comment:
       prefer Collections.emptyMap()

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {
+    try {
+      pluginInfos = 
ContainerPluginsApi.plugins(coreContainer.zkClientSupplier);
+    } catch (IOException e) {
+      log.error("Could not read plugins data", e);
+      return;
+    }
+    if(pluginInfos.isEmpty()) return;
+
+    for (Map.Entry<String, Object> e : pluginInfos.entrySet()) {
+      PluginMeta info = null;
+      try {
+        info = mapper.readValue(Utils.toJSON(e.getValue()), PluginMeta.class);
+      } catch (IOException ioException) {
+        log.error("Invalid apiInfo configuration :", ioException);
+      }
+
+      ApiInfo apiInfo = null;
+      try {
+        List<String> errs = new ArrayList<>();
+        apiInfo = new ApiInfo(info, errs);
+        if (!errs.isEmpty()) {
+          log.error(StrUtils.join(errs, ','));
+          continue;
+        }
+      } catch (Exception ex) {
+        log.error("unable to instantiate apiInfo ", ex);
+        continue;
+      }
+
+      String path = pluginNameVsPath.get(e.getKey());
+      if (path == null) {
+        // there is a new apiInfo . let's register it
+        try {
+          apiInfo.init();
+          ApiHolder holder = new ApiHolder(apiInfo);
+          plugins.put(holder.key, holder);
+          pluginNameVsPath.put(apiInfo.info.name, holder.key);
+          containerApiBag.register(holder, Collections.EMPTY_MAP);
+        } catch (Exception exp) {
+          log.error("Cannot install apiInfo ", exp);
+        }
+      } else {
+        ApiHolder old = plugins.get(apiInfo.key);
+        if (path.equals(apiInfo.key)) {
+          if (Objects.equals(info.version, old.apiInfo.info.version)) {
+            //this plugin uses the same version. No need to update
+            continue;
+          }
+          //this apiInfo existed at the same path but uses a newer version of 
the package
+          //refresh the existing Api holder
+          try {
+            apiInfo.init();
+          } catch (Exception exception) {
+            log.error("COuld not inititlaize Plugin", exception);
+          }
+          plugins.get(apiInfo.key).refresh(apiInfo);
+        } else {// the path is changed for the same apiInfo. it's not allowed
+          log.error("Cannot register the same apiInfo at a different path old 
path: " + path + "new path : " + apiInfo.key);
+        }
+      }
+    }
+    Set<String> toBeRemoved = new HashSet<>();
+    for (String s : pluginNameVsPath.keySet()) {
+      if (!pluginInfos.containsKey(s)) {
+        toBeRemoved.add(s);
+      }
+    }
+    for (String s : toBeRemoved) {
+      String pathKey = pluginNameVsPath.remove(s);
+      ApiHolder holder = plugins.remove(pathKey);
+      if (holder != null) {
+        Api old = 
containerApiBag.unregister(holder.apiInfo.endPoint.method()[0], 
holder.apiInfo.endPoint.path()[0]);
+        if (old instanceof Closeable) {
+          closeWhileHandlingException((Closeable) old);
+        }
+
+      }
+    }
+  }
+
+  private class ApiHolder extends Api {
+    private final String key;
+    private ApiInfo apiInfo;
+
+    protected ApiHolder(ApiInfo apiInfo) throws Exception {
+      super(apiInfo.delegate);
+      this.apiInfo = apiInfo;
+      this.key = apiInfo.key;
+    }
+
+    @Override
+    public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
+      apiInfo.delegate.call(req, rsp);
+    }
+
+    void refresh(ApiInfo info) {
+      this.apiInfo = info;
+      super.spec = info.delegate;
+    }
+  }
+
+  public class ApiInfo implements ReflectMapWriter  {
+    /*This is the path at which this handler is
+     *
+     */
+    @JsonProperty
+    public String key;
+    @JsonProperty
+    private final PluginMeta info;
+
+    @JsonProperty(value = "package")
+    public final String pkg;
+    private PackageLoader.Package.Version pkgVersion;
+    EndPoint endPoint;
+
+    private Class klas;
+
+
+    private AnnotatedApi delegate;
+
+
+    public ApiInfo(PluginMeta info, List<String> errs) {

Review comment:
       This feels like a super klunky API writing errors to a string in a 
constructor argument? Why can't we just throw exceptions here?

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {
+    try {
+      pluginInfos = 
ContainerPluginsApi.plugins(coreContainer.zkClientSupplier);
+    } catch (IOException e) {
+      log.error("Could not read plugins data", e);
+      return;
+    }
+    if(pluginInfos.isEmpty()) return;
+
+    for (Map.Entry<String, Object> e : pluginInfos.entrySet()) {
+      PluginMeta info = null;
+      try {
+        info = mapper.readValue(Utils.toJSON(e.getValue()), PluginMeta.class);
+      } catch (IOException ioException) {
+        log.error("Invalid apiInfo configuration :", ioException);
+      }
+
+      ApiInfo apiInfo = null;
+      try {
+        List<String> errs = new ArrayList<>();
+        apiInfo = new ApiInfo(info, errs);
+        if (!errs.isEmpty()) {
+          log.error(StrUtils.join(errs, ','));
+          continue;
+        }
+      } catch (Exception ex) {
+        log.error("unable to instantiate apiInfo ", ex);
+        continue;
+      }
+
+      String path = pluginNameVsPath.get(e.getKey());
+      if (path == null) {
+        // there is a new apiInfo . let's register it
+        try {
+          apiInfo.init();
+          ApiHolder holder = new ApiHolder(apiInfo);
+          plugins.put(holder.key, holder);
+          pluginNameVsPath.put(apiInfo.info.name, holder.key);
+          containerApiBag.register(holder, Collections.EMPTY_MAP);
+        } catch (Exception exp) {
+          log.error("Cannot install apiInfo ", exp);
+        }
+      } else {
+        ApiHolder old = plugins.get(apiInfo.key);
+        if (path.equals(apiInfo.key)) {
+          if (Objects.equals(info.version, old.apiInfo.info.version)) {
+            //this plugin uses the same version. No need to update
+            continue;
+          }
+          //this apiInfo existed at the same path but uses a newer version of 
the package
+          //refresh the existing Api holder
+          try {
+            apiInfo.init();
+          } catch (Exception exception) {
+            log.error("COuld not inititlaize Plugin", exception);
+          }
+          plugins.get(apiInfo.key).refresh(apiInfo);
+        } else {// the path is changed for the same apiInfo. it's not allowed
+          log.error("Cannot register the same apiInfo at a different path old 
path: " + path + "new path : " + apiInfo.key);
+        }
+      }
+    }
+    Set<String> toBeRemoved = new HashSet<>();
+    for (String s : pluginNameVsPath.keySet()) {
+      if (!pluginInfos.containsKey(s)) {
+        toBeRemoved.add(s);
+      }
+    }
+    for (String s : toBeRemoved) {
+      String pathKey = pluginNameVsPath.remove(s);
+      ApiHolder holder = plugins.remove(pathKey);
+      if (holder != null) {
+        Api old = 
containerApiBag.unregister(holder.apiInfo.endPoint.method()[0], 
holder.apiInfo.endPoint.path()[0]);
+        if (old instanceof Closeable) {
+          closeWhileHandlingException((Closeable) old);
+        }
+
+      }
+    }
+  }
+
+  private class ApiHolder extends Api {
+    private final String key;
+    private ApiInfo apiInfo;
+
+    protected ApiHolder(ApiInfo apiInfo) throws Exception {
+      super(apiInfo.delegate);
+      this.apiInfo = apiInfo;
+      this.key = apiInfo.key;
+    }
+
+    @Override
+    public void call(SolrQueryRequest req, SolrQueryResponse rsp) {
+      apiInfo.delegate.call(req, rsp);
+    }
+
+    void refresh(ApiInfo info) {
+      this.apiInfo = info;
+      super.spec = info.delegate;
+    }
+  }
+
+  public class ApiInfo implements ReflectMapWriter  {
+    /*This is the path at which this handler is
+     *
+     */
+    @JsonProperty
+    public String key;
+    @JsonProperty
+    private final PluginMeta info;
+
+    @JsonProperty(value = "package")
+    public final String pkg;
+    private PackageLoader.Package.Version pkgVersion;
+    EndPoint endPoint;
+
+    private Class klas;

Review comment:
       nit: raw types

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {

Review comment:
       This parameter is unused?

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {
+    try {
+      pluginInfos = 
ContainerPluginsApi.plugins(coreContainer.zkClientSupplier);
+    } catch (IOException e) {
+      log.error("Could not read plugins data", e);
+      return;
+    }
+    if(pluginInfos.isEmpty()) return;
+
+    for (Map.Entry<String, Object> e : pluginInfos.entrySet()) {
+      PluginMeta info = null;
+      try {
+        info = mapper.readValue(Utils.toJSON(e.getValue()), PluginMeta.class);
+      } catch (IOException ioException) {
+        log.error("Invalid apiInfo configuration :", ioException);

Review comment:
       should this have a `continue`?

##########
File path: 
solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -0,0 +1,178 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.handler.admin;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+import org.apache.solr.api.AnnotatedApi;
+import org.apache.solr.api.Command;
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.api.EndPoint;
+import org.apache.solr.api.PayloadObj;
+import org.apache.solr.client.solrj.SolrRequest.METHOD;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.security.PermissionNameProvider;
+import org.apache.zookeeper.KeeperException;
+import org.apache.zookeeper.data.Stat;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+
+public class ContainerPluginsApi {
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  public static final String PLUGIN = "plugin";
+  private final Supplier<SolrZkClient> zkClientSupplier;
+  private final CoreContainer coreContainer;
+  public final Read readAPI = new Read();
+  public final Edit editAPI = new Edit();
+
+  public ContainerPluginsApi(CoreContainer coreContainer) {
+    this.zkClientSupplier = coreContainer.zkClientSupplier;
+    this.coreContainer = coreContainer;
+  }
+
+  @EndPoint(method = METHOD.GET,
+      path = "/cluster/plugin",
+      permission = PermissionNameProvider.Name.COLL_READ_PERM)
+  public class Read {
+
+    @Command
+    public void list(SolrQueryRequest req, SolrQueryResponse rsp) throws 
IOException {
+      rsp.add(PLUGIN, plugins(zkClientSupplier));
+    }
+  }
+
+  @EndPoint(method = METHOD.POST,
+      path = "/cluster/plugin",
+      permission = PermissionNameProvider.Name.COLL_EDIT_PERM)
+  public class Edit {
+
+    @Command(name = "add")
+    public void add(SolrQueryRequest req, SolrQueryResponse rsp, 
PayloadObj<PluginMeta> payload) throws IOException {
+      PluginMeta info = payload.get();
+      validateConfig(payload, info);
+      if(payload.hasError()) return;
+      persistPlugins(map -> {
+        if (map.containsKey(info.name)) {
+          payload.addError(info.name + " already exists");
+          return null;
+        }
+        map.put(info.name, info);
+        return map;
+      });
+    }
+
+    @Command(name = "remove")
+    public void remove(SolrQueryRequest req, SolrQueryResponse rsp, 
PayloadObj<String> payload) throws IOException {
+      persistPlugins(map -> {
+        if (map.remove(payload.get()) == null) {
+          payload.addError("No such plugin: " + payload.get());
+          return null;
+        }
+        return map;
+      });
+    }
+
+    @Command(name = "update")
+    public void update(SolrQueryRequest req, SolrQueryResponse rsp, 
PayloadObj<PluginMeta> payload) throws IOException {
+      PluginMeta info = payload.get();
+      validateConfig(payload, info);
+      if(payload.hasError()) return;
+      persistPlugins(map -> {
+        Map existing = (Map) map.get(info.name);
+        if (existing == null) {
+          payload.addError("No such plugin: " + info.name);
+          return null;
+        } else {
+          map.put(info.name, info);
+          return map;
+        }
+      });
+    }
+  }
+
+  private void validateConfig(PayloadObj<PluginMeta> payload, PluginMeta info) 
{
+    if (info.klass.indexOf(':') > 0) {
+      if (info.version == null) {
+        payload.addError("Using package. must provide a packageVersion");
+        return;
+      }
+    }
+    List<String> errs = new ArrayList<>();
+    CustomContainerPlugins.ApiInfo apiInfo = 
coreContainer.getCustomContainerPlugins().createInfo(info, errs);
+    if (!errs.isEmpty()) {
+      for (String err : errs) payload.addError(err);
+      return;
+    }
+    AnnotatedApi api = null ;
+    try {
+      api =  apiInfo.init();
+    } catch (Exception e) {
+      log.error("Error instantiating plugin ", e);
+      errs.add(e.getMessage());
+      return;
+    } finally {
+      closeWhileHandlingException(api);
+    }
+  }
+
+  public static Map<String, Object> plugins(Supplier<SolrZkClient> 
zkClientSupplier) throws IOException {
+    SolrZkClient zkClient = zkClientSupplier.get();
+    try {
+      Map<String, Object> clusterPropsJson = (Map<String, Object>) 
Utils.fromJSON(zkClient.getData(ZkStateReader.CLUSTER_PROPS, null, new Stat(), 
true));
+      return (Map<String, Object>) clusterPropsJson.computeIfAbsent(PLUGIN, 
Utils.NEW_LINKED_HASHMAP_FUN);
+    } catch (KeeperException.NoNodeException e) {
+      return new LinkedHashMap<>();
+    } catch (KeeperException | InterruptedException e) {
+      throw new IOException("Error reading cluster property", 
SolrZkClient.checkInterrupted(e));
+    }
+  }
+
+  private void persistPlugins(Function<Map, Map> modifier) throws IOException {

Review comment:
       nit: raw types on Map

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {

Review comment:
       there's a lot of nested classes here, can any of those be split out into 
their own files?

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);
+    return false;
+  }
+
+  @Override
+  public void writeMap(EntryWriter ew) {
+    plugins.forEach((s, apiHolder) -> ew.putNoEx(s, apiHolder.apiInfo));
+  }
+
+  public CustomContainerPlugins(CoreContainer coreContainer, ApiBag apiBag) {
+    this.coreContainer = coreContainer;
+    this.containerApiBag = apiBag;
+  }
+
+  public void refresh(Map<String, Object> pluginInfos) {
+    try {
+      pluginInfos = 
ContainerPluginsApi.plugins(coreContainer.zkClientSupplier);
+    } catch (IOException e) {
+      log.error("Could not read plugins data", e);
+      return;
+    }
+    if(pluginInfos.isEmpty()) return;
+
+    for (Map.Entry<String, Object> e : pluginInfos.entrySet()) {
+      PluginMeta info = null;
+      try {
+        info = mapper.readValue(Utils.toJSON(e.getValue()), PluginMeta.class);
+      } catch (IOException ioException) {
+        log.error("Invalid apiInfo configuration :", ioException);
+      }
+
+      ApiInfo apiInfo = null;
+      try {
+        List<String> errs = new ArrayList<>();
+        apiInfo = new ApiInfo(info, errs);
+        if (!errs.isEmpty()) {
+          log.error(StrUtils.join(errs, ','));
+          continue;
+        }
+      } catch (Exception ex) {
+        log.error("unable to instantiate apiInfo ", ex);
+        continue;
+      }
+
+      String path = pluginNameVsPath.get(e.getKey());
+      if (path == null) {
+        // there is a new apiInfo . let's register it
+        try {
+          apiInfo.init();
+          ApiHolder holder = new ApiHolder(apiInfo);
+          plugins.put(holder.key, holder);
+          pluginNameVsPath.put(apiInfo.info.name, holder.key);
+          containerApiBag.register(holder, Collections.EMPTY_MAP);
+        } catch (Exception exp) {
+          log.error("Cannot install apiInfo ", exp);
+        }
+      } else {
+        ApiHolder old = plugins.get(apiInfo.key);
+        if (path.equals(apiInfo.key)) {
+          if (Objects.equals(info.version, old.apiInfo.info.version)) {
+            //this plugin uses the same version. No need to update
+            continue;
+          }
+          //this apiInfo existed at the same path but uses a newer version of 
the package
+          //refresh the existing Api holder
+          try {
+            apiInfo.init();
+          } catch (Exception exception) {
+            log.error("COuld not inititlaize Plugin", exception);
+          }
+          plugins.get(apiInfo.key).refresh(apiInfo);
+        } else {// the path is changed for the same apiInfo. it's not allowed
+          log.error("Cannot register the same apiInfo at a different path old 
path: " + path + "new path : " + apiInfo.key);
+        }
+      }
+    }
+    Set<String> toBeRemoved = new HashSet<>();
+    for (String s : pluginNameVsPath.keySet()) {
+      if (!pluginInfos.containsKey(s)) {
+        toBeRemoved.add(s);
+      }
+    }
+    for (String s : toBeRemoved) {

Review comment:
       why do we need to iterate twice here? 

##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -0,0 +1,283 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.api;
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Modifier;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.solr.client.solrj.request.beans.PluginMeta;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.annotation.JsonProperty;
+import org.apache.solr.common.cloud.ClusterPropertiesListener;
+import org.apache.solr.common.util.Pair;
+import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.common.util.StrUtils;
+import org.apache.solr.common.util.Utils;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+import org.apache.solr.pkg.PackageLoader;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.SolrJacksonAnnotationInspector;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;
+
+public class CustomContainerPlugins implements MapWriter, 
ClusterPropertiesListener {
+  private ObjectMapper mapper = 
SolrJacksonAnnotationInspector.createObjectMapper();
+  private static final Logger log = 
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  final CoreContainer coreContainer;
+  final ApiBag containerApiBag;
+  private Map<String, ApiHolder> plugins = new HashMap<>();
+  private Map<String, String> pluginNameVsPath = new HashMap<>();
+
+  @Override
+  public boolean onChange(Map<String, Object> properties) {
+    refresh(null);

Review comment:
       `refresh(properties)`?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to