dsmiley commented on code in PR #2126: URL: https://github.com/apache/solr/pull/2126#discussion_r1420976112
########## solr/core/src/java/org/apache/solr/api/ZkContainerPluginsSource.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.api; + +import java.io.IOException; +import java.lang.invoke.MethodHandles; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.function.Function; +import java.util.function.Supplier; +import org.apache.solr.client.solrj.request.beans.PluginMeta; +import org.apache.solr.common.cloud.SolrZkClient; +import org.apache.solr.common.cloud.ZkStateReader; +import org.apache.solr.common.util.Utils; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.handler.admin.ContainerPluginsApi; +import org.apache.zookeeper.KeeperException; +import org.apache.zookeeper.data.Stat; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The plugin configurations are stored and retrieved from the ZooKeeper cluster properties This Review Comment: Should state were specifically the data lives in ZK. ########## solr/core/src/java/org/apache/solr/api/ContainerPluginsSource.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.api; + +import java.io.IOException; +import java.util.Map; +import java.util.function.Function; +import org.apache.solr.handler.admin.ContainerPluginsApi; + +/** A source for Container Plugin configurations */ +public interface ContainerPluginsSource { + + /** + * Get the Container Plugins Read Api for this plugin source + * + * @return A {@link ContainerPluginsApi} Read Api for this plugin source + */ + ContainerPluginsApi.Read getReadApi(); + + /** + * Get the Container Plugins Edit Api for this plugin source, if it supports edit operations + * + * @return A {@link ContainerPluginsApi} Edit Api for this plugin source, or null if the plugin + * source does not support editing the plugin configs + */ + ContainerPluginsApi.Edit getEditApi(); + + /** Get the Container plugin configurations from this source */ + Map<String, Object> plugins() throws IOException; + + /** + * Persist the updated set of plugin configs + * + * @param modifier A function providing the map of plugin configs to be persisted + */ + void persistPlugins(Function<Map<String, Object>, Map<String, Object>> modifier) Review Comment: a very confusing method (to me) but I presume this is as it was so I won't overly critique this ########## solr/core/src/java/org/apache/solr/core/CoreContainer.java: ########## @@ -1098,9 +1104,13 @@ private void loadInternal() { if (isZooKeeperAware()) { containerPluginsRegistry.refresh(); getZkController().zkStateReader.registerClusterPropertiesListener(containerPluginsRegistry); - ContainerPluginsApi containerPluginsApi = new ContainerPluginsApi(this); - registerV2ApiIfEnabled(containerPluginsApi.readAPI); - registerV2ApiIfEnabled(containerPluginsApi.editAPI); + if (pluginsSource.getReadApi() != null) { + registerV2ApiIfEnabled(pluginsSource.getReadApi()); Review Comment: maybe null should be an acceptable arg ########## solr/core/src/test/org/apache/solr/api/NodeConfigContainerPluginsSourceTest.java: ########## @@ -0,0 +1,287 @@ +/* + * 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.api; + +import static org.apache.solr.client.solrj.SolrRequest.METHOD.GET; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.client.solrj.impl.BaseHttpSolrClient; +import org.apache.solr.client.solrj.request.V2Request; +import org.apache.solr.client.solrj.request.beans.PluginMeta; +import org.apache.solr.client.solrj.response.V2Response; +import org.apache.solr.cloud.ClusterSingleton; +import org.apache.solr.cloud.MiniSolrCloudCluster; +import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.MapWriter; +import org.apache.solr.common.annotation.JsonProperty; +import org.apache.solr.common.util.ReflectMapWriter; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.request.SolrQueryRequest; +import org.apache.solr.response.SolrQueryResponse; +import org.apache.solr.security.PermissionNameProvider; +import org.junit.BeforeClass; +import org.junit.Test; + +/** Tests that verify initialization of container plugins that are declared in solr.xml */ +public class NodeConfigContainerPluginsSourceTest extends SolrCloudTestCase { + + private static final String NODE_CONFIG_PLUGINS_SOURCE_XML = + "<containerPluginsSource class=\"org.apache.solr.api.NodeConfigContainerPluginsSource\"/>"; + + @BeforeClass + public static void setupCluster() throws Exception { + configureCluster(1) + .withSolrXml( + MiniSolrCloudCluster.DEFAULT_CLOUD_SOLR_XML.replace( + "</solr>", + NODE_CONFIG_PLUGINS_SOURCE_XML + SingletonNoConfig.xmlConfig() + "</solr>")) + .addConfig( + "conf", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf")) + .configure(); + } + + /** Verifies that a cluster singleton config declared in solr.xml is loaded into the registry */ + public void testOneClusterSingleton() { + CoreContainer cc = newCoreContainer(solrXml(SingletonNoConfig.xmlConfig())); Review Comment: Can you please consider using EmbeddedSolrServerTestRule instead of newCoreContainer? newCoreContainer isn't yet deprecated but it's related to Solr's oldest tests using TestHarness that I'm trying to get rid of. I don't recall ever seen a SolrCloudTestCase subclass calling that. Usually, *either* the test uses SolrCloud as managed by SolrCloudTestCase *or* it not a SolrCloud test at all. ########## solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java: ########## @@ -647,6 +650,17 @@ private static PluginInfo[] getBackupRepositoryPluginInfos(List<ConfigNode> cfg) return configs; } + private static PluginInfo[] getClusterSingletonPluginInfos(List<ConfigNode> nodes) { + if (nodes == null || nodes.isEmpty()) { Review Comment: a java streams approach might be more elegant ########## solr/core/src/java/org/apache/solr/api/ContainerPluginsSource.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.api; + +import java.io.IOException; +import java.util.Map; +import java.util.function.Function; +import org.apache.solr.handler.admin.ContainerPluginsApi; + +/** A source for Container Plugin configurations */ +public interface ContainerPluginsSource { + + /** + * Get the Container Plugins Read Api for this plugin source + * + * @return A {@link ContainerPluginsApi} Read Api for this plugin source + */ + ContainerPluginsApi.Read getReadApi(); + + /** + * Get the Container Plugins Edit Api for this plugin source, if it supports edit operations + * + * @return A {@link ContainerPluginsApi} Edit Api for this plugin source, or null if the plugin + * source does not support editing the plugin configs + */ + ContainerPluginsApi.Edit getEditApi(); + + /** Get the Container plugin configurations from this source */ Review Comment: Mutable or not? -- 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
