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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
