gerlowskija commented on code in PR #2060: URL: https://github.com/apache/solr/pull/2060#discussion_r1385226940
########## solr/CHANGES.txt: ########## @@ -480,6 +480,9 @@ Improvements * SOLR-16397: Unload core v2 endpoints have been updated to be more REST-ful. UNLOAD is now available at `POST /api/cores/coreName/unload` (Sanjay Dutt via Jason Gerlowski) +* SOLR-16397: Swap core v2 endpoints have been updated to be more REST-ful. Review Comment: [-1] Oops - I think this update is in the wrong section. It's in the "Improvements" section (👍 ) but for the 9.3 release (👎 ). 9.4 came out a few weeks ago, so the first release this change would make it into would be 9.5 There should be a similar "Improvements" section for the 9.5 release - could you move this entry there? ########## solr/core/src/java/org/apache/solr/handler/admin/api/SwapCores.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.security.PermissionNameProvider.Name.CORE_EDIT_PERM; + +import javax.inject.Inject; +import org.apache.solr.client.api.endpoint.SwapCoresApi; +import org.apache.solr.client.api.model.SolrJerseyResponse; +import org.apache.solr.client.api.model.SwapCoresRequestBody; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.CoreAdminHandler; +import org.apache.solr.jersey.PermissionName; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; + +public class SwapCores extends CoreAdminAPIBase implements SwapCoresApi { + @Inject + public SwapCores( + CoreContainer coreContainer, + CoreAdminHandler.CoreAdminAsyncTracker coreAdminAsyncTracker, + SolrQueryRequest solrQueryRequest, + SolrQueryResponse solrQueryResponse) { + super(coreContainer, coreAdminAsyncTracker, solrQueryRequest, solrQueryResponse); + } + + @PermissionName(CORE_EDIT_PERM) + @Override + public SolrJerseyResponse swapCores(String coreName, final SwapCoresRequestBody requestBody) + throws Exception { + ensureRequiredParameterProvided("coreName", coreName); + if (requestBody == null || requestBody.with == null) { Review Comment: [-1] Maybe there's a particular reason you structured things this way (that I'm just missing). But if not: let's get rid of the if-statement here. The `ensureBlah` methods have conditionals internally, so we should be able to do all the arg-validation with: ``` ensureRequiredParameterProvided("coreName", coreName); ensureRequiredRequestBodyProvided(requestBody); ensureRequiredParameterProvided("with", requestBody.with); ``` ########## solr/solr-ref-guide/modules/configuration-guide/pages/coreadmin-api.adoc: ########## @@ -390,7 +390,31 @@ This can be used to swap new content into production. The prior core remains available and can be swapped back, if necessary. Each core will be known by the name of the other, after the swap. +[.dynamic-tabs] Review Comment: [+1] Woohoo, nice doc addition! ########## solr/CHANGES.txt: ########## @@ -480,6 +480,9 @@ Improvements * SOLR-16397: Unload core v2 endpoints have been updated to be more REST-ful. UNLOAD is now available at `POST /api/cores/coreName/unload` (Sanjay Dutt via Jason Gerlowski) +* SOLR-16397: Swap core v2 endpoints have been updated to be more REST-ful. Review Comment: [-1] Oops - I think this update is in the wrong section. It's in the "Improvements" section (👍 ) but for the 9.3 release (👎 ). 9.4 came out a few weeks ago, so the first release this change would make it into would be 9.5 There should be a similar "Improvements" section for the 9.5 release - could you move this entry there? -- 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]
