gerlowskija commented on code in PR #1126: URL: https://github.com/apache/solr/pull/1126#discussion_r1016898284
########## solr/core/src/test/org/apache/solr/handler/admin/api/CoreSnapshotAPITest.java: ########## @@ -0,0 +1,161 @@ +/* + * 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.util.ArrayList; +import java.util.List; +import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.junit.After; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +public class CoreSnapshotAPITest extends SolrTestCaseJ4 { Review Comment: [+1] These tests look awesome! Nice to avoid heavy mocking and also avoid standing up a full SolrCloud cluster or anything similarly-heavy like that. ########## solr/core/src/java/org/apache/solr/handler/admin/api/CoreAdminAPIBase.java: ########## @@ -0,0 +1,210 @@ +/* + * 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.util.Collections; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.function.Supplier; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.api.V2ApiUtils; +import org.apache.solr.jersey.SolrJerseyResponse; +import org.apache.solr.logging.MDCLoggingContext; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.util.tracing.TraceUtils; +import org.slf4j.MDC; + +public abstract class CoreAdminAPIBase extends JerseyResource { Review Comment: [0] Small nit, should add Javadocs explaining what this base class handles for callers and when/how it should be used. ########## solr/core/src/java/org/apache/solr/handler/admin/api/CoreAdminAPIBase.java: ########## @@ -0,0 +1,210 @@ +/* + * 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.util.Collections; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.concurrent.ExecutorService; +import java.util.function.Supplier; +import org.apache.solr.api.JerseyResource; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.api.V2ApiUtils; +import org.apache.solr.jersey.SolrJerseyResponse; +import org.apache.solr.logging.MDCLoggingContext; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.util.tracing.TraceUtils; +import org.slf4j.MDC; + +public abstract class CoreAdminAPIBase extends JerseyResource { + + private static final ExecutorService PARALLEL_EXECUTOR = Review Comment: [-1] I love the approach on this class overall (esp. the `handle` method that takes in a Supplier), but I'm concerned about reliance on static state here. I hinted at this a bit on JIRA, but should've been more explicit: > I guess we could create a base-class for our APIs, e.g. "AsyncTrackingApiBase", with a static instance to hold this status info, that gets extended by various API subclasses. I don't love the idea of introducing non-constant static state, but I think it'd work? It's my fault for not being more explicit: it's not the norm, but it's also not unheard of to run multiple Solr instances from within the same JVM. Solr's tests do it, for one, and I've heard of actual applications doing this too on occasion via EmbeddedSolrServer and similar classes. Maybe this would "just work" if the tests and EmbeddedSolrServer know to use different classloaders, but if it doesn't, then the use of static state could cause tricky bugs - e.g. the same executor being used for multiple Solr instances. I'll run the test suite and poke around some of the related code to double-check my fears here, but assuming they're well founded, we'll need a different approach here. One approach that's only slightly a "dodge" would be to essentially re-use the same status-map and `parallelExecutor` that CoreAdminHandler creates. (i.e. Add getters for these things to `CoreAdminHandler` and then modify the Jersey application to make the status-map and executor injectable as singletons, similar to what we do for CoreContainer [here](https://github.com/apache/solr/blob/c86128855807042e25c5bd320e6f7729784e8884/solr/core/src/java/org/apache/solr/core/CoreContainer.java#L1067)) This would have a few side-benefits: - avoids the creation of an additional executor (the current PR creates one in the handler and one in this API class) - no executor lifecycle changes required, since we're reusing the executor - injection gives the API classes enough indirection that they won't require any changes when we eventually nuke v1 and need to find a different way of managing the executor/status-map - no duplication of constants and status-map init logic (current PR has this code in both the handler and this API class). Any thoughts on that? I'm happy to make the changes required as the rework is largely my fault for not communicating my concerns around static-state strongly enough. -- 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]
