gerlowskija commented on code in PR #2235: URL: https://github.com/apache/solr/pull/2235#discussion_r1483286529
########## solr/api/src/java/org/apache/solr/client/api/endpoint/CreateCoreApi.java: ########## @@ -0,0 +1,37 @@ +/* + * 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.client.api.endpoint; + +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; +import org.apache.solr.client.api.model.CreateCoreRequestBody; +import org.apache.solr.client.api.model.SolrJerseyResponse; + +/** + * V2 API for creating a new core on the receiving node. + * + * <p>This API (POST /api/cores {...}) is analogous to the v1 /admin/cores?action=CREATE command. + * + * @see CreateCoreRequestBody + */ +@Path("/cores/{coreName}/create") Review Comment: [-1] To be more consistent with our other APIs, I think this should be something a little more like: `POST /api/cores {...}`. This would bring it into line with Solr's other "creation" endpoints: - collection creaton (`POST /api/collections {...}`) - shard creation (`POST /collections/someCollName/shards {...}`) - alias creation (`POST /api/aliases {...}`) - etc. We do sometimes put explicit "verbs" in the HTTP path (like you've done with 'create' here), but I think we've been trying to reserve those for operations that don't map well onto existing HTTP methods. (i.e. `POST` is used pretty often for creation operations, so hopefully the API semantics are clear without putting 'create' in the path explicitly) ########## solr/core/src/java/org/apache/solr/handler/admin/CoreAdminHandler.java: ########## @@ -380,7 +352,7 @@ public Collection<Api> getApis() { final List<Api> apis = new ArrayList<>(); apis.addAll(AnnotatedApi.getApis(new AllCoresStatusAPI(this))); apis.addAll(AnnotatedApi.getApis(new SingleCoreStatusAPI(this))); - apis.addAll(AnnotatedApi.getApis(new CreateCoreAPI(this))); + // apis.addAll(AnnotatedApi.getApis(new CreateCoreAPI(this))); Review Comment: [0] Commenting here as a reminder to delete this line as we get closer to merge. ########## solr/core/src/test/org/apache/solr/handler/admin/api/CreateCoreApiTest.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.api; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.client.api.model.CreateCoreRequestBody; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.CoreAdminHandler; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +/** Test for {@link CreateCore} */ +public class CreateCoreApiTest extends SolrTestCaseJ4 { + private CoreContainer coreContainer; + private CreateCore createCore; + private final String CREATE_CORE_NAME = "demo1"; + private Path instDir; + private CoreAdminHandler coreAdminHandler; + + @BeforeClass + public static void beforeClass() throws Exception { + initCore("solrconfig.xml", "schema.xml"); + } + + public String getCoreWorkDir() { + return this.getClass().getName(); + } + + @Override + @Before + public void setUp() throws Exception { + super.setUp(); + final Path workDir = createTempDir(getCoreWorkDir()); + instDir = workDir.resolve(CREATE_CORE_NAME); + Path subHome = instDir.resolve("conf"); + Files.createDirectories(subHome); + String srcDir = SolrTestCaseJ4.TEST_HOME() + "/collection1/conf"; + Files.copy(Path.of(srcDir, "schema-tiny.xml"), subHome.resolve("schema_ren.xml")); + Files.copy(Path.of(srcDir, "solrconfig-minimal.xml"), subHome.resolve("solrconfig_ren.xml")); + Files.copy( + Path.of(srcDir, "solrconfig.snippet.randomindexconfig.xml"), + subHome.resolve("solrconfig.snippet.randomindexconfig.xml")); + final CoreContainer cores = h.getCoreContainer(); + cores.getAllowPaths().add(workDir); + // cores.getAllowPaths().add(workDir); + coreAdminHandler = new CoreAdminHandler(cores); + SolrQueryResponse response = new SolrQueryResponse(); + createCore = + new CreateCore( + coreAdminHandler.getCoreContainer(), + coreAdminHandler.getCoreAdminAsyncTracker(), + req(), + response); + } + + @Test + public void test_createCore_with_existent_configSet() { + CreateCoreRequestBody createCoreRequestBody = new CreateCoreRequestBody(); + createCoreRequestBody.name = coreName; + createCoreRequestBody.configSet = "_default"; + createCoreRequestBody.dataDir = "data_demo"; + createCoreRequestBody.instanceDir = instDir.toAbsolutePath().toString(); + createCore.createCore(createCoreRequestBody, CREATE_CORE_NAME); + Path dataDir = instDir.resolve("data_demo"); + assertTrue(Files.exists(dataDir)); + } + + @Test + public void testReportError_createCore_non_existent_configSet() { + CreateCoreRequestBody createCoreRequestBody = new CreateCoreRequestBody(); + createCoreRequestBody.name = coreName; + createCoreRequestBody.configSet = "_default_non_existent_configSet"; + createCoreRequestBody.instanceDir = instDir.toAbsolutePath().toString(); + assertThrows( + "Could not load configuration from directory", + SolrException.class, + () -> createCore.createCore(createCoreRequestBody, CREATE_CORE_NAME)); + } + + @Test + public void testReportError_two_thread_simultaneously_createCore() throws InterruptedException { + final CreateCoreRequestBody createCoreRequestBody = new CreateCoreRequestBody(); + createCoreRequestBody.name = coreName; + createCoreRequestBody.instanceDir = instDir.toAbsolutePath().toString(); + createCoreRequestBody.config = "solrconfig_ren.xml"; + createCoreRequestBody.schema = "schema_ren.xml"; + createCoreRequestBody.dataDir = "data_demo"; + final AtomicReference<Exception> exp = new AtomicReference<>(); + final Runnable coreCreationCmd = + () -> { + try { + createCore.createCore(createCoreRequestBody, CREATE_CORE_NAME); + } catch (Exception e) { + exp.set(e); + } + }; + + Thread firstCoreCreationThread = new Thread(coreCreationCmd); + Thread secCoreCreationThread = new Thread(coreCreationCmd); + firstCoreCreationThread.start(); Review Comment: [0] Might simplify this code slightly to create a ExecutorService via `ExecutorUtil`, rather than directly managing threads? Just a suggestion... Also you're likely aware of this, but this setup doesn't guarantee that the core creations happen "simultaneously". "First" might be finished by the time that "second" is started. It's still a good test to have IMO, just might need a different name. ########## solr/core/src/java/org/apache/solr/handler/admin/api/CreateCore.java: ########## @@ -0,0 +1,146 @@ +/* + * 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.api; + +import static org.apache.solr.common.params.CollectionAdminParams.PROPERTY_PREFIX; +import static org.apache.solr.handler.api.V2ApiUtils.flattenMapWithPrefix; +import static org.apache.solr.handler.api.V2ApiUtils.flattenToCommaDelimitedString; + +import com.fasterxml.jackson.core.type.TypeReference; +import com.fasterxml.jackson.databind.ObjectMapper; +import jakarta.inject.Inject; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import org.apache.solr.client.api.endpoint.CreateCoreApi; +import org.apache.solr.client.api.model.CreateCoreRequestBody; +import org.apache.solr.client.api.model.CreateCoreResponseBody; +import org.apache.solr.client.api.model.SolrJerseyResponse; +import org.apache.solr.cloud.ZkController; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.common.util.PropertiesUtil; +import org.apache.solr.common.util.StrUtils; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.CoreAdminHandler; +import org.apache.solr.jersey.PermissionName; +import org.apache.solr.jersey.SolrJacksonMapper; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.security.PermissionNameProvider; + +/** + * Implementation of V2 api {@link CreateCoreApi} for creating core. + * + * @see CreateCoreRequestBody + * @see CoreAdminAPIBase + */ +public class CreateCore extends CoreAdminAPIBase implements CreateCoreApi { + private final ObjectMapper objectMapper; + + @Inject + public CreateCore( + CoreContainer coreContainer, + CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker, + SolrQueryRequest req, + SolrQueryResponse rsp) { + super(coreContainer, coreAdminAsyncTracker, req, rsp); + this.objectMapper = SolrJacksonMapper.getObjectMapper(); + } + + @Override + @PermissionName(PermissionNameProvider.Name.CORE_EDIT_PERM) + public SolrJerseyResponse createCore( + CreateCoreRequestBody createCoreRequestBody, String coreName) { + final Map<String, Object> requestBodyMap = Review Comment: [0] I wrote and have used `Utils.reflectToMap()` to do similar things for other Solr APIs. I'm not an expert on either Jackson or reflection - so I can't really tell whether this is the same or different, better or worse than how `reflectToMap` works. Do you happen to know? If using Jackson directly in this way is better, maybe it'd make sense to reimplement `reflectToMap` to do the better thing and we could call it here. Alternately if you just weren't aware of `Utils.reflectToMap`, maybe the quickest path forward is to just use reflectToWrite here, and then we can tackle evaluating and potentially reimplementing `Utils.reflectToMap` as a separate JIRA ticket later. ########## solr/core/src/java/org/apache/solr/core/CoreContainer.java: ########## @@ -2588,15 +2585,13 @@ public void addKeyVal(Object map, Object key, Object val) throws IOException { } class CloserThread extends Thread { - CoreContainer container; - SolrCores solrCores; - NodeConfig cfg; + private final CoreContainer container; + private final SolrCores solrCores; - CloserThread(CoreContainer container, SolrCores solrCores, NodeConfig cfg) { + CloserThread(CoreContainer container, SolrCores solrCores) { Review Comment: [Q] Is this a small opportunistic "cleanup" of an unused instance variable? Or does it have some relevance to this PR that I'm not seeing? It looks small and innocuous, but I've been burned before in my own PRs by small cleanups that I thought couldn't possibly cause any problems 😛 I don't mind being opportunistic here and there, but be careful! (The extra "noise" in the PR diff can sometimes also make it harder for folks less familiar with these sort of API changes to review) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
