gerlowskija commented on code in PR #1933:
URL: https://github.com/apache/solr/pull/1933#discussion_r1331534623
##########
solr/solr-ref-guide/modules/configuration-guide/pages/coreadmin-api.adoc:
##########
@@ -439,7 +439,31 @@ The `UNLOAD` action removes a core from Solr.
Active requests will continue to be processed, but no new requests will be
sent to the named core.
If a core is registered under more than one name, only the given name is
removed.
-`admin/cores?action=UNLOAD&core=_core-name_`
+[.dynamic-tabs]
Review Comment:
[+1] Thanks for remembering the docs, looks great!
##########
solr/core/src/java/org/apache/solr/handler/admin/api/UnloadCoreAPI.java:
##########
@@ -17,60 +17,89 @@
package org.apache.solr.handler.admin.api;
-import static org.apache.solr.client.solrj.SolrRequest.METHOD.POST;
-import static org.apache.solr.handler.ClusterAPI.wrapParams;
+import static
org.apache.solr.client.solrj.impl.BinaryResponseParser.BINARY_CONTENT_TYPE_V2;
import static
org.apache.solr.security.PermissionNameProvider.Name.CORE_EDIT_PERM;
-import java.util.HashMap;
-import java.util.Locale;
-import java.util.Map;
-import org.apache.solr.api.Command;
-import org.apache.solr.api.EndPoint;
-import org.apache.solr.api.PayloadObj;
+import io.swagger.v3.oas.annotations.media.Schema;
+import io.swagger.v3.oas.annotations.parameters.RequestBody;
+import javax.inject.Inject;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
+import org.apache.solr.client.api.model.SolrJerseyResponse;
import org.apache.solr.common.annotation.JsonProperty;
-import org.apache.solr.common.params.CoreAdminParams;
-import org.apache.solr.common.util.ReflectMapWriter;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.CoreDescriptor;
import org.apache.solr.handler.admin.CoreAdminHandler;
+import org.apache.solr.jersey.JacksonReflectMapWriter;
+import org.apache.solr.jersey.PermissionName;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.apache.solr.util.TestInjection;
/**
- * V2 API for renaming an existing Solr core.
+ * V2 API for unloading an existing Solr core.
*
- * <p>The new API (POST /v2/cores/coreName {'unload': {...}}) is equivalent to
the v1
- * /admin/cores?action=unload command.
+ * <p>The new API (POST /v2/cores/coreName/unload is equivalent to the v1
/admin/cores?action=unload
+ * command.
*/
-@EndPoint(
- path = {"/cores/{core}"},
- method = POST,
- permission = CORE_EDIT_PERM)
-public class UnloadCoreAPI {
- private static final String V2_UNLOAD_CORE_CMD = "unload";
+@Path("/cores/{coreName}/unload")
+public class UnloadCoreAPI extends CoreAdminAPIBase {
- private final CoreAdminHandler coreHandler;
-
- public UnloadCoreAPI(CoreAdminHandler coreHandler) {
- this.coreHandler = coreHandler;
+ @Inject
+ public UnloadCoreAPI(
+ CoreContainer coreContainer,
+ CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker,
+ SolrQueryRequest solrQueryRequest,
+ SolrQueryResponse solrQueryResponse) {
+ super(coreContainer, coreAdminAsyncTracker, solrQueryRequest,
solrQueryResponse);
}
- @Command(name = V2_UNLOAD_CORE_CMD)
- public void unloadCore(PayloadObj<UnloadCorePayload> obj) throws Exception {
- final UnloadCorePayload v2Body = obj.get();
- final Map<String, Object> v1Params = v2Body.toMap(new HashMap<>());
- v1Params.put(
- CoreAdminParams.ACTION,
-
CoreAdminParams.CoreAdminAction.UNLOAD.name().toLowerCase(Locale.ROOT));
- v1Params.put(
- CoreAdminParams.CORE,
obj.getRequest().getPathTemplateValues().get(CoreAdminParams.CORE));
-
- coreHandler.handleRequestBody(wrapParams(obj.getRequest(), v1Params),
obj.getResponse());
+ @POST
+ @Produces({"application/json", "application/xml", BINARY_CONTENT_TYPE_V2})
+ @PermissionName(CORE_EDIT_PERM)
+ public SolrJerseyResponse unloadCore(
+ @PathParam("coreName") String coreName,
+ @Schema(description = "The POJO for representing additional Unload core
params") @RequestBody
+ UnloadCoreRequestBody unloadCoreRequestBody)
+ throws Exception {
+ ensureRequiredParameterProvided("coreName", coreName);
+ SolrJerseyResponse solrJerseyResponse =
instantiateJerseyResponse(SolrJerseyResponse.class);
+ return handlePotentiallyAsynchronousTask(
+ solrJerseyResponse,
+ coreName,
+ unloadCoreRequestBody.async,
+ "unload",
+ () -> {
+ CoreDescriptor cdescr = coreContainer.getCoreDescriptor(coreName);
+ coreContainer.unload(
+ coreName,
+ unloadCoreRequestBody.deleteIndex,
+ unloadCoreRequestBody.deleteDataDir,
+ unloadCoreRequestBody.deleteInstanceDir);
+ assert
TestInjection.injectNonExistentCoreExceptionAfterUnload(coreName);
+ return solrJerseyResponse;
+ });
}
- public static class UnloadCorePayload implements ReflectMapWriter {
- @JsonProperty public Boolean deleteIndex;
+ public static class UnloadCoreRequestBody implements JacksonReflectMapWriter
{
+ @Schema(description = "If true, will remove the index when unloading the
core.")
+ @JsonProperty(defaultValue = "false")
+ public boolean deleteIndex;
Review Comment:
[-0] Jackson's support for filing in default values is really nice and makes
the primitive types tempting (over their class-based counterparts like
`Boolean`, etc.). But one downside of having Jackson inject defaults this way
is that Solr has no way to tell whether a given value was actually specified by
an end user, or whether it was injected as a default.
This rarely matters, but there are a few cases where it does. For example:
request logging. Solr prints out a log message for each request, and has
historically only shown the user-provided params (and not defaults). That's
not really possible with Jackson injecting defaults, afaict.
I'd love to find a clean way around this and a few other edge cases where it
matters (e.g. code generation), but for the interim we're stuck using
`Boolean`, etc. Would you be open to changing these properties to `Boolean`
type? The default-val insertion could then be done with inline-if's
(`requestBody.deleteIndex == null ? false : requestBody.deleteIndex`) on L78-80.
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java:
##########
@@ -118,14 +120,18 @@ public enum CoreAdminOperation implements CoreAdminOp {
it -> {
SolrParams params = it.req.getParams();
String cname = params.required().get(CoreAdminParams.CORE);
-
- boolean deleteIndexDir = params.getBool(CoreAdminParams.DELETE_INDEX,
false);
- boolean deleteDataDir =
params.getBool(CoreAdminParams.DELETE_DATA_DIR, false);
- boolean deleteInstanceDir =
params.getBool(CoreAdminParams.DELETE_INSTANCE_DIR, false);
- CoreDescriptor cdescr =
it.handler.coreContainer.getCoreDescriptor(cname);
- it.handler.coreContainer.unload(cname, deleteIndexDir, deleteDataDir,
deleteInstanceDir);
-
- assert TestInjection.injectNonExistentCoreExceptionAfterUnload(cname);
+ UnloadCoreAPI.UnloadCoreRequestBody unloadCoreRequestBody =
Review Comment:
[0] David Smiley has pointed out to me a few times now that lines like this
really benefit from the `var` syntax/keyword that was introduced in Java 10.
e.g.
`final UnloadCoreAPI.UnloadCoreRequestBody requestBody = new
UnloadCoreAPI.UnloadCoreRequestBody();`
vs
`final var requestBody = new UnloadCoreAPI.UnloadCoreRequestBody();`
----
I don't care strongly either way, but figured I'd share the tip in case it
resonates with you.
--
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
