[GitHub] [solr] cpoerschke commented on a diff in pull request #931: SOLR-16282: Update core admin handler to use pluggable custom actions

[GitBox <gi...@apache.org>]

cpoerschke commented on code in PR #931:
URL: https://github.com/apache/solr/pull/931#discussion_r921391560


##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -190,7 +212,7 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw
 
       // Pick the action
       final String action = req.getParams().get(ACTION, STATUS.toString()).toLowerCase(Locale.ROOT);
-      CoreAdminOperation op = opMap.get(action);
+      CoreAdminOp op = opMap.get(action);
       if (op == null) {
         handleCustomAction(req, rsp);

Review Comment:
   minor: maybe we could INFO log the `action` here i.e. the `@Deprecated` annotation on `handleCustomAction` method shows that custom function might be used and operationally logging here would show that/when/how the custom function is used
   ```suggestion
           log.info("action '{}' not found, calling custom action handler", action);
           handleCustomAction(req, rsp);
   ```



##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -154,6 +155,27 @@ public Boolean registerV2() {
     return Boolean.TRUE;
   }
 
+  /**
+   * Registers custom actions defined in {@code solr.xml}. Called from the {@link CoreContainer}
+   * during load process.
+   */
+  public void registerCustomActions(Map<String, CoreAdminOp> customActions) {

Review Comment:
   ```suggestion
     public final void registerCustomActions(Map<String, CoreAdminOp> customActions) {
   ```



##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -105,6 +106,8 @@ public class NodeConfig {
 
   private final PluginInfo tracerConfig;
 
+  private final Map<String, String> coreAdminHandlerActions;

Review Comment:
   minor: perhaps add after `private final String coreAdminHandlerClass;` since they are related, and similar in other places below



##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -132,6 +133,14 @@ public static NodeConfig fromConfig(Path solrHome, XmlConfigFile config, boolean
     String nodeName = (String) entries.remove("nodeName");
     if (Strings.isNullOrEmpty(nodeName) && cloudConfig != null) nodeName = cloudConfig.getHost();
 
+    Map<String, String> coreAdminHandlerActions =
+        readNodeListAsNamedList(
+                config, "solr/coreAdminHandlerActions/*[@name]", "<coreAdminHandlerActions>")
+            .asMap()
+            .entrySet()
+            .stream()
+            .collect(Collectors.toMap(item -> item.getKey(), item -> item.getValue().toString()));
+

Review Comment:
   observation: conceptually this fits in the `fillSolrSection` method but that has only `entries` available as its argument and hence this is placed here



##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java:
##########
@@ -399,19 +424,16 @@ public void shutdown() {
     if (parallelExecutor != null) ExecutorUtil.shutdownAndAwaitTermination(parallelExecutor);
   }
 
-  private static final Map<String, CoreAdminOperation> opMap = new HashMap<>();
+  private static final Map<String, CoreAdminOp> opMap = new HashMap<>();
 
-  static class CallInfo {
-    final CoreAdminHandler handler;
-    final SolrQueryRequest req;
-    final SolrQueryResponse rsp;
-    final CoreAdminOperation op;
+  public static class CallInfo {
+    public final CoreAdminHandler handler;
+    public final SolrQueryRequest req;
+    public final SolrQueryResponse rsp;
+    public final CoreAdminOp op;
 
     CallInfo(
-        CoreAdminHandler handler,
-        SolrQueryRequest req,
-        SolrQueryResponse rsp,
-        CoreAdminOperation op) {
+        CoreAdminHandler handler, SolrQueryRequest req, SolrQueryResponse rsp, CoreAdminOp op) {

Review Comment:
   observations:
   * making the class and its elements public is backwards compatible I'd say
   * `public enum CoreAdminOperation implements CoreAdminOp {` is the case already and so the signature change from `CoreAdminOperation` to `CoreAdminOp` is also backwards compatible



##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2157,6 +2169,12 @@ public void waitForLoadingCore(String name, long timeoutMs) {
     solrCores.waitForLoadingCoreToFinish(name, timeoutMs);
   }
 
+  // ---------------- Core admin handler operations --------------
+
+  protected <A> A createCoreAdminHandlerOperation(String operationClass, Class<A> clazz) {
+    return loader.newInstance(operationClass, clazz, null, null, null);

Review Comment:
   There's a two-arg `loader.newInstance` variant it seems, wondering if that could be used instead here, or if not leave a comment w.r.t. why this variant is used. Though if the two-arg variant is usable perhaps `createCoreAdminHandlerOperation` would not be needed?



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org