dsmiley commented on code in PR #2438: URL: https://github.com/apache/solr/pull/2438#discussion_r1628632286
########## solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.core; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.rest.RestManager; + +/** + * A synthetic core that is created only in memory and not registered against Zookeeper. + * + * <p>This is only used in Coordinator node to support a subset of SolrCore functionalities required + * by Coordinator operations such as aggregating and writing out response and providing configset + * info. + * + * <p>There should only be one instance of SyntheticSolrCore per configset + */ +public class SyntheticSolrCore extends SolrCore { + public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) { + super(coreContainer, cd, configSet); + } + + public static SyntheticSolrCore createAndRegisterCore( + CoreContainer coreContainer, String syntheticCoreName, String configSetName) { + Map<String, String> coreProps = new HashMap<>(); Review Comment: Would `Map.of` work? ########## solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.core; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.rest.RestManager; + +/** + * A synthetic core that is created only in memory and not registered against Zookeeper. + * + * <p>This is only used in Coordinator node to support a subset of SolrCore functionalities required + * by Coordinator operations such as aggregating and writing out response and providing configset + * info. + * + * <p>There should only be one instance of SyntheticSolrCore per configset + */ +public class SyntheticSolrCore extends SolrCore { + public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) { + super(coreContainer, cd, configSet); + } + + public static SyntheticSolrCore createAndRegisterCore( + CoreContainer coreContainer, String syntheticCoreName, String configSetName) { + Map<String, String> coreProps = new HashMap<>(); + coreProps.put(CoreAdminParams.CORE_NODE_NAME, coreContainer.getHostName()); + coreProps.put(CoreAdminParams.COLLECTION, syntheticCoreName); + + CoreDescriptor syntheticCoreDescriptor = + new CoreDescriptor( + syntheticCoreName, + Paths.get(coreContainer.getSolrHome() + "/" + syntheticCoreName), Review Comment: Path & String slash concatenation together is a sad thing; misses a value proposition of the Path API. Someday in main branch, ought to finally switch getSolrHome to return a Path but nonetheless you can do Paths.of(..).resolve(syntheticCoreName) for example. ########## solr/core/src/java/org/apache/solr/servlet/CoordinatorHttpSolrCall.java: ########## @@ -88,198 +81,70 @@ public static SolrCore getCore( String syntheticCoreName = factory.collectionVsCoreNameMapping.get(collectionName); if (syntheticCoreName != null) { SolrCore syntheticCore = solrCall.cores.getCore(syntheticCoreName); - setMDCLoggingContext(collectionName); + setMdcLoggingContext(collectionName); return syntheticCore; } else { + // first time loading this collection ZkStateReader zkStateReader = solrCall.cores.getZkController().getZkStateReader(); ClusterState clusterState = zkStateReader.getClusterState(); DocCollection coll = clusterState.getCollectionOrNull(collectionName, true); - SolrCore core = null; - if (coll != null) { - String confName = coll.getConfigName(); - String syntheticCollectionName = getSyntheticCollectionName(confName); - DocCollection syntheticColl = clusterState.getCollectionOrNull(syntheticCollectionName); - synchronized (CoordinatorHttpSolrCall.class) { - if (syntheticColl == null) { - // no synthetic collection for this config, let's create one - if (log.isInfoEnabled()) { - log.info( - "synthetic collection: {} does not exist, creating.. ", syntheticCollectionName); - } - - SolrException createException = null; - try { - createColl(syntheticCollectionName, solrCall.cores, confName); - } catch (SolrException exception) { - // concurrent requests could have created the collection hence causing collection - // exists - // exception - createException = exception; - } finally { - syntheticColl = - zkStateReader.getClusterState().getCollectionOrNull(syntheticCollectionName); - } - - // then indeed the collection was not created properly, either by this or other - // concurrent - // requests - if (syntheticColl == null) { - if (createException != null) { - throw createException; // rethrow the exception since such collection was not - // created - } else { - throw new SolrException( - SolrException.ErrorCode.SERVER_ERROR, - "Could not locate synthetic collection [" - + syntheticCollectionName - + "] after creation!"); - } - } - } - - // get docCollection again to ensure we get the fresh state - syntheticColl = - zkStateReader.getClusterState().getCollectionOrNull(syntheticCollectionName); - List<Replica> nodeNameSyntheticReplicas = - syntheticColl.getReplicas(solrCall.cores.getZkController().getNodeName()); - if (nodeNameSyntheticReplicas == null || nodeNameSyntheticReplicas.isEmpty()) { - // this node does not have a replica. add one - if (log.isInfoEnabled()) { - log.info( - "this node does not have a replica of the synthetic collection: {} , adding replica ", - syntheticCollectionName); - } + if (coll == null) { // querying on a non-existent collection, it could have been removed + log.info( + "Cannot find collection {} to proxy call to, it could have been deleted", + collectionName); + return null; + } - addReplica(syntheticCollectionName, solrCall.cores); - } + String confName = coll.getConfigName(); + syntheticCoreName = getSyntheticCoreName(confName); + + SolrCore syntheticCore; + synchronized (CoordinatorHttpSolrCall.class) { + CoreContainer coreContainer = solrCall.cores; + syntheticCore = coreContainer.getCore(syntheticCoreName); + if (syntheticCore == null) { + // first time loading this config set + log.info("Loading synthetic core for config set {}", confName); + syntheticCore = + SyntheticSolrCore.createAndRegisterCore( + coreContainer, syntheticCoreName, coll.getConfigName()); + // getting the core should open it + syntheticCore.open(); + } - // still have to ensure that it's active, otherwise super.getCoreByCollection - // will return null and then CoordinatorHttpSolrCall will call getCore again - // hence creating a calling loop - try { - zkStateReader.waitForState( - syntheticCollectionName, - 10, - TimeUnit.SECONDS, - docCollection -> { - List<Replica> replicas = - docCollection.getReplicas(solrCall.cores.getZkController().getNodeName()); - if (replicas == null || replicas.isEmpty()) { + factory.collectionVsCoreNameMapping.put(collectionName, syntheticCore.getName()); + + // for the watcher, only remove on collection deletion (ie collection == null), since + // watch from coordinator is collection specific + coreContainer + .getZkController() + .getZkStateReader() + .registerDocCollectionWatcher( + collectionName, + collection -> { + if (collection == null) { + factory.collectionVsCoreNameMapping.remove(collectionName); + return true; + } else { return false; } - for (Replica nodeNameSyntheticReplica : replicas) { - if (nodeNameSyntheticReplica.getState() == Replica.State.ACTIVE) { - return true; - } - } - return false; }); - } catch (Exception e) { - throw new SolrException( - SolrException.ErrorCode.SERVER_ERROR, - "Failed to wait for active replica for synthetic collection [" - + syntheticCollectionName - + "]", - e); - } - } - - core = solrCall.getCoreByCollection(syntheticCollectionName, isPreferLeader); - if (core != null) { - factory.collectionVsCoreNameMapping.put(collectionName, core.getName()); - // for the watcher, only remove on collection deletion (ie collection == null), since - // watch from coordinator is collection specific - solrCall - .cores - .getZkController() - .getZkStateReader() - .registerDocCollectionWatcher( - collectionName, - collection -> { - if (collection == null) { - factory.collectionVsCoreNameMapping.remove(collectionName); - return true; - } else { - return false; - } - }); - if (log.isDebugEnabled()) { - log.debug("coordinator node, returns synthetic core: {}", core.getName()); - } - } - setMDCLoggingContext(collectionName); - return core; } - return null; + setMdcLoggingContext(collectionName); + if (log.isDebugEnabled()) { + log.debug("coordinator node, returns synthetic core: {}", syntheticCore.getName()); + } + return syntheticCore; } } public static String getSyntheticCollectionName(String configName) { return SYNTHETIC_COLL_PREFIX + configName; } - /** - * Overrides the MDC context as the core set was synthetic core, which does not reflect the - * collection being operated on - */ - private static void setMDCLoggingContext(String collectionName) { - MDCLoggingContext.setCollection(collectionName); - - // below is irrelevant for call to coordinator - MDCLoggingContext.setCoreName(null); - MDCLoggingContext.setShard(null); - MDCLoggingContext.setCoreName(null); - } - - private static void addReplica(String syntheticCollectionName, CoreContainer cores) { - SolrQueryResponse rsp = new SolrQueryResponse(); - try { - CollectionAdminRequest.AddReplica addReplicaRequest = - CollectionAdminRequest.addReplicaToShard(syntheticCollectionName, "shard1") - // we are fixing the name, so that no two replicas are created in the same node - .setNode(cores.getZkController().getNodeName()); - addReplicaRequest.setWaitForFinalState(true); - cores - .getCollectionsHandler() - .handleRequestBody(new LocalSolrQueryRequest(null, addReplicaRequest.getParams()), rsp); - if (rsp.getValues().get("success") == null) { - throw new SolrException( - SolrException.ErrorCode.SERVER_ERROR, - "Could not auto-create collection: " + Utils.toJSONString(rsp.getValues())); - } - } catch (Exception e) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); - } - } - - private static void createColl( - String syntheticCollectionName, CoreContainer cores, String confName) { - SolrQueryResponse rsp = new SolrQueryResponse(); - try { - CollectionAdminRequest.Create collCreationRequest = - CollectionAdminRequest.createCollection(syntheticCollectionName, confName, 1, 1) - .setCreateNodeSet(cores.getZkController().getNodeName()); - collCreationRequest.setWaitForFinalState(true); - SolrParams params = collCreationRequest.getParams(); - if (log.isInfoEnabled()) { - log.info("sending collection admin command : {}", Utils.toJSONString(params)); - } - cores.getCollectionsHandler().handleRequestBody(new LocalSolrQueryRequest(null, params), rsp); - if (rsp.getValues().get("success") == null) { - throw new SolrException( - SolrException.ErrorCode.SERVER_ERROR, - "Could not create :" - + syntheticCollectionName - + " collection: " - + Utils.toJSONString(rsp.getValues())); - } - } catch (SolrException e) { - throw e; - - } catch (Exception e) { - throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); - } + public static String getSyntheticCoreName(String configName) { Review Comment: From a pure type signature perspective (thus erasure of "configName" name), this signature could be confusing. The string I'd expect to give such a method would be the collection name but no it's not that. Maybe "FromConfig" suffix? ########## solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.core; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.rest.RestManager; + +/** + * A synthetic core that is created only in memory and not registered against Zookeeper. + * + * <p>This is only used in Coordinator node to support a subset of SolrCore functionalities required + * by Coordinator operations such as aggregating and writing out response and providing configset + * info. + * + * <p>There should only be one instance of SyntheticSolrCore per configset + */ +public class SyntheticSolrCore extends SolrCore { + public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) { + super(coreContainer, cd, configSet); + } + + public static SyntheticSolrCore createAndRegisterCore( + CoreContainer coreContainer, String syntheticCoreName, String configSetName) { + Map<String, String> coreProps = new HashMap<>(); + coreProps.put(CoreAdminParams.CORE_NODE_NAME, coreContainer.getHostName()); Review Comment: is this necessary; why is the host name in the core descriptor as it's not really related to the core (it's at a different level) ########## solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.core; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.rest.RestManager; + +/** + * A synthetic core that is created only in memory and not registered against Zookeeper. + * + * <p>This is only used in Coordinator node to support a subset of SolrCore functionalities required + * by Coordinator operations such as aggregating and writing out response and providing configset + * info. + * + * <p>There should only be one instance of SyntheticSolrCore per configset + */ +public class SyntheticSolrCore extends SolrCore { + public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) { + super(coreContainer, cd, configSet); + } + + public static SyntheticSolrCore createAndRegisterCore( + CoreContainer coreContainer, String syntheticCoreName, String configSetName) { + Map<String, String> coreProps = new HashMap<>(); + coreProps.put(CoreAdminParams.CORE_NODE_NAME, coreContainer.getHostName()); + coreProps.put(CoreAdminParams.COLLECTION, syntheticCoreName); + + CoreDescriptor syntheticCoreDescriptor = + new CoreDescriptor( + syntheticCoreName, + Paths.get(coreContainer.getSolrHome() + "/" + syntheticCoreName), + coreProps, + coreContainer.getContainerProperties(), + coreContainer.getZkController()); + syntheticCoreDescriptor.setConfigSet(configSetName); + ConfigSet coreConfig = + coreContainer.getConfigSetService().loadConfigSet(syntheticCoreDescriptor); + syntheticCoreDescriptor.setConfigSetTrusted(coreConfig.isTrusted()); + SyntheticSolrCore syntheticCore = + new SyntheticSolrCore(coreContainer, syntheticCoreDescriptor, coreConfig); + coreContainer.registerCore(syntheticCoreDescriptor, syntheticCore, false, false); + + return syntheticCore; + } + + @Override + protected void bufferUpdatesIfConstructing(CoreDescriptor coreDescriptor) { + // no updates to SyntheticSolrCore + } + + @Override + protected RestManager initRestManager() throws SolrException { + // returns an initialized RestManager. As init routines requires reading configname of the + // core's collection from ZK + // which synthetic core is not registered in ZK. + // We do not expect RestManager ops on Coordinator Nodes Review Comment: Rewrap. Glad to see this kind of comment. Hmm; I wonder if it's actually prevented? Like how does this overall feature (which I confess knowing nothing about) actually prevent these cores from being used for _anything_ except the one thing it needs to exist for -- distributed-search? e.g. maybe. no handlers should be loaded except those extending SearchHandler? ########## solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.core; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.CoreAdminParams; +import org.apache.solr.rest.RestManager; + +/** + * A synthetic core that is created only in memory and not registered against Zookeeper. + * + * <p>This is only used in Coordinator node to support a subset of SolrCore functionalities required + * by Coordinator operations such as aggregating and writing out response and providing configset + * info. + * + * <p>There should only be one instance of SyntheticSolrCore per configset + */ +public class SyntheticSolrCore extends SolrCore { + public SyntheticSolrCore(CoreContainer coreContainer, CoreDescriptor cd, ConfigSet configSet) { + super(coreContainer, cd, configSet); + } + + public static SyntheticSolrCore createAndRegisterCore( + CoreContainer coreContainer, String syntheticCoreName, String configSetName) { + Map<String, String> coreProps = new HashMap<>(); + coreProps.put(CoreAdminParams.CORE_NODE_NAME, coreContainer.getHostName()); + coreProps.put(CoreAdminParams.COLLECTION, syntheticCoreName); + + CoreDescriptor syntheticCoreDescriptor = + new CoreDescriptor( + syntheticCoreName, + Paths.get(coreContainer.getSolrHome() + "/" + syntheticCoreName), + coreProps, + coreContainer.getContainerProperties(), + coreContainer.getZkController()); + syntheticCoreDescriptor.setConfigSet(configSetName); + ConfigSet coreConfig = + coreContainer.getConfigSetService().loadConfigSet(syntheticCoreDescriptor); + syntheticCoreDescriptor.setConfigSetTrusted(coreConfig.isTrusted()); + SyntheticSolrCore syntheticCore = + new SyntheticSolrCore(coreContainer, syntheticCoreDescriptor, coreConfig); + coreContainer.registerCore(syntheticCoreDescriptor, syntheticCore, false, false); + + return syntheticCore; + } + + @Override + protected void bufferUpdatesIfConstructing(CoreDescriptor coreDescriptor) { + // no updates to SyntheticSolrCore + } + + @Override + protected RestManager initRestManager() throws SolrException { + // returns an initialized RestManager. As init routines requires reading configname of the + // core's collection from ZK + // which synthetic core is not registered in ZK. + // We do not expect RestManager ops on Coordinator Nodes + return new RestManager(); + } + + @Override + public void close() { Review Comment: remove; right? ########## solr/core/src/java/org/apache/solr/cloud/ZkConfigSetService.java: ########## @@ -71,36 +68,21 @@ public ZkConfigSetService(SolrZkClient zkClient) { @Override public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) { - final String colName = cd.getCollectionName(); - - // For back compat with cores that can create collections without the collections API - try { - if (!zkClient.exists(ZkStateReader.COLLECTIONS_ZKNODE + "/" + colName, true)) { - // TODO remove this functionality or maybe move to a CLI mechanism - log.warn( - "Auto-creating collection (in ZK) from core descriptor (on disk). This feature may go away!"); - CreateCollectionCmd.createCollectionZkNode( - zkController.getSolrCloudManager().getDistribStateManager(), - colName, - cd.getCloudDescriptor().getParams(), - zkController.getCoreContainer().getConfigSetService()); - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new ZooKeeperException( - SolrException.ErrorCode.SERVER_ERROR, "Interrupted auto-creating collection", e); - } catch (KeeperException e) { - throw new ZooKeeperException( - SolrException.ErrorCode.SERVER_ERROR, "Failure auto-creating collection", e); - } - // The configSet is read from ZK and populated. Ignore CD's pre-existing configSet; only Review Comment: > Ignore CD's pre-existing configSet Wrong, now. -- 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
