sigram commented on a change in pull request #1684: URL: https://github.com/apache/lucene-solr/pull/1684#discussion_r467801413
########## File path: solr/core/src/java/org/apache/solr/cluster/placement/Cluster.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.cluster.placement; + +import java.io.IOException; +import java.util.Optional; +import java.util.Set; + +/** + * <p>A representation of the (initial) cluster state, providing information on which nodes are part of the cluster and a way + * to get to more detailed info. + * + * <p>This instance can also be used as a {@link PropertyValueSource} if {@link PropertyKey}'s need to be specified with + * a global cluster target. + */ +public interface Cluster extends PropertyValueSource { + /** + * @return current set of live nodes. Never <code>null</code>, never empty (Solr wouldn't call the plugin if empty + * since no useful work could then be done). + */ + Set<Node> getLiveNodes(); + + /** + * <p>Returns info about the given collection if one exists. Because it is not expected for plugins to request info about + * a large number of collections, requests can only be made one by one. + * + * <p>This is also the reason we do not return a {@link java.util.Map} or {@link Set} of {@link SolrCollection}'s here: it would be + * wasteful to fetch all data and fill such a map when plugin code likely needs info about at most one or two collections. + */ + Optional<SolrCollection> getCollection(String collectionName) throws IOException; + + /** + * <p>Allows getting all {@link SolrCollection} present in the cluster. + * + * <p><b>WARNING:</b> this call might be extremely inefficient on large clusters. Usage is discouraged. + */ + Set<SolrCollection> getAllCollections(); Review comment: I meant just the list of names ... `Collection<String>`, otherwise I agree it can be very inefficient. ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/PlacementPlanFactory.java ########## @@ -0,0 +1,52 @@ +/* + * 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.cluster.placement; + +import java.util.Set; + +/** + * Allows plugins to create {@link PlacementPlan}s telling the Solr layer where to create replicas following the processing of + * a {@link PlacementRequest}. The Solr layer can (and will) check that the {@link PlacementPlan} conforms to the {@link PlacementRequest} (and + * if it does not, the requested operation will fail). + */ +public interface PlacementPlanFactory { + /** + * <p>Creates a {@link PlacementPlan} for adding a new collection and its replicas. + * + * <p>This is in support of {@link org.apache.solr.cloud.api.collections.CreateCollectionCmd}. + */ + PlacementPlan createPlacementPlanNewCollection(CreateNewCollectionPlacementRequest request, String CollectionName, Set<ReplicaPlacement> replicaPlacements); + + /** + * <p>Creates a {@link PlacementPlan} for adding replicas to a given shard of an existing collection. + * + * <p>This is in support (directly or indirectly) of {@link org.apache.solr.cloud.api.collections.AddReplicaCmd}, + * {@link org.apache.solr.cloud.api.collections.CreateShardCmd}, {@link org.apache.solr.cloud.api.collections.ReplaceNodeCmd}, + * {@link org.apache.solr.cloud.api.collections.MoveReplicaCmd}, {@link org.apache.solr.cloud.api.collections.SplitShardCmd}, + * {@link org.apache.solr.cloud.api.collections.RestoreCmd} and {@link org.apache.solr.cloud.api.collections.MigrateCmd}. + * (as well as of {@link org.apache.solr.cloud.api.collections.CreateCollectionCmd} in the specific case of + * {@link org.apache.solr.common.params.CollectionAdminParams#WITH_COLLECTION} but this should be removed shortly and + * the section in parentheses of this comment should be removed when the {@code withCollection} javadoc link appears broken). + */ + PlacementPlan createPlacementPlanAddReplicas(AddReplicasPlacementRequest request, String CollectionName, Set<ReplicaPlacement> replicaPlacements); Review comment: Should we include MOVEREPLICA here, too? Some placement plans need to know in advance whether it's an unrelated ADD or a part of MOVE operation - eg. if the replica files are on a shared FS then MOVE is not equivalent to ADD + DELETE because the actual files are not being moved. So I think we need both `createAddReplicas` and `createMoveReplicas` and `createDeleteReplicas`. ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/PlacementRequest.java ########## @@ -0,0 +1,28 @@ +/* + * 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.cluster.placement; + +/** + * A cluster related placement request that Solr asks a {@link PlacementPlugin} plugin to resolve and compute a {@link PlacementPlan} for. + */ +public interface PlacementRequest { + /** + * "Unique" request ID that can be used for logging in the plugin code and that will also be used in logs on the Solr side. + */ + String getUniqueRequestId(); Review comment: `getRequestId`. Javadoc can clarify that it's unique. ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/PlacementPlanFactory.java ########## @@ -0,0 +1,52 @@ +/* + * 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.cluster.placement; + +import java.util.Set; + +/** + * Allows plugins to create {@link PlacementPlan}s telling the Solr layer where to create replicas following the processing of + * a {@link PlacementRequest}. The Solr layer can (and will) check that the {@link PlacementPlan} conforms to the {@link PlacementRequest} (and + * if it does not, the requested operation will fail). + */ +public interface PlacementPlanFactory { + /** + * <p>Creates a {@link PlacementPlan} for adding a new collection and its replicas. + * + * <p>This is in support of {@link org.apache.solr.cloud.api.collections.CreateCollectionCmd}. + */ + PlacementPlan createPlacementPlanNewCollection(CreateNewCollectionPlacementRequest request, String CollectionName, Set<ReplicaPlacement> replicaPlacements); + + /** + * <p>Creates a {@link PlacementPlan} for adding replicas to a given shard of an existing collection. + * + * <p>This is in support (directly or indirectly) of {@link org.apache.solr.cloud.api.collections.AddReplicaCmd}, + * {@link org.apache.solr.cloud.api.collections.CreateShardCmd}, {@link org.apache.solr.cloud.api.collections.ReplaceNodeCmd}, + * {@link org.apache.solr.cloud.api.collections.MoveReplicaCmd}, {@link org.apache.solr.cloud.api.collections.SplitShardCmd}, + * {@link org.apache.solr.cloud.api.collections.RestoreCmd} and {@link org.apache.solr.cloud.api.collections.MigrateCmd}. + * (as well as of {@link org.apache.solr.cloud.api.collections.CreateCollectionCmd} in the specific case of + * {@link org.apache.solr.common.params.CollectionAdminParams#WITH_COLLECTION} but this should be removed shortly and + * the section in parentheses of this comment should be removed when the {@code withCollection} javadoc link appears broken). + */ + PlacementPlan createPlacementPlanAddReplicas(AddReplicasPlacementRequest request, String CollectionName, Set<ReplicaPlacement> replicaPlacements); + + /** + * Creates a {@link ReplicaPlacement} needed to be passed to some/all {@link PlacementPlan} factory methods. + */ + ReplicaPlacement createReplicaPlacement(String shardName, Node node, Replica.ReplicaType replicaType); Review comment: Do we need to expose this as a separate public method? I would expect this to be an internal detail of the implementation. ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/PlacementPlugin.java ########## @@ -0,0 +1,41 @@ +/* + * 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.cluster.placement; + +/** + * Implemented by external plugins to control replica placement and movement on the search cluster (as well as other things + * such as cluster elasticity?) when cluster changes are required (initiated elsewhere, most likely following a Collection + * API call). + */ +public interface PlacementPlugin { Review comment: > get on that map, very low cost here as well Oh, I wasn't worried about that .. My point was that the plugin may need to initialize costly internal structures based on the config (like fetching some additional params from an external system to eg. verify license constraints or cost schedule for allocating additional resources). ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/SamplePluginMinimizeCores.java ########## @@ -0,0 +1,132 @@ +/* + * 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.cluster.placement.plugins; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Iterator; +import java.util.Set; +import java.util.Map; + +import com.google.common.collect.Ordering; +import com.google.common.collect.TreeMultimap; +import org.apache.solr.cluster.placement.Cluster; +import org.apache.solr.cluster.placement.CoresCountPropertyValue; +import org.apache.solr.cluster.placement.CreateNewCollectionPlacementRequest; +import org.apache.solr.cluster.placement.Node; +import org.apache.solr.cluster.placement.PlacementException; +import org.apache.solr.cluster.placement.PlacementPlugin; +import org.apache.solr.cluster.placement.PropertyKey; +import org.apache.solr.cluster.placement.PropertyKeyFactory; +import org.apache.solr.cluster.placement.PropertyValue; +import org.apache.solr.cluster.placement.PropertyValueFetcher; +import org.apache.solr.cluster.placement.Replica; +import org.apache.solr.cluster.placement.ReplicaPlacement; +import org.apache.solr.cluster.placement.PlacementRequest; +import org.apache.solr.cluster.placement.PlacementPlan; +import org.apache.solr.cluster.placement.PlacementPlanFactory; +import org.apache.solr.common.util.SuppressForbidden; + +/** + * Implements placing replicas to minimize number of cores per {@link Node}, while not placing two replicas of the same + * shard on the same node. + * + * TODO: code not tested and never run, there are no implementation yet for used interfaces + */ +public class SamplePluginMinimizeCores implements PlacementPlugin { + + @SuppressForbidden(reason = "Ordering.arbitrary() has no equivalent in Comparator class. Rather reuse than copy.") + public PlacementPlan computePlacement(Cluster cluster, PlacementRequest placementRequest, PropertyKeyFactory propertyFactory, + PropertyValueFetcher propertyFetcher, PlacementPlanFactory placementPlanFactory) throws PlacementException { + // This plugin only supports Creating a collection. + if (!(placementRequest instanceof CreateNewCollectionPlacementRequest)) { + throw new PlacementException("This toy plugin only supports creating collections"); + } + + final CreateNewCollectionPlacementRequest reqCreateCollection = (CreateNewCollectionPlacementRequest) placementRequest; + + final int totalReplicasPerShard = reqCreateCollection.getNrtReplicationFactor() + + reqCreateCollection.getTlogReplicationFactor() + reqCreateCollection.getPullReplicationFactor(); + + if (cluster.getLiveNodes().size() < totalReplicasPerShard) { + throw new PlacementException("Cluster size too small for number of replicas per shard"); + } + + // Get number of cores on each Node + TreeMultimap<Integer, Node> nodesByCores = TreeMultimap.create(Comparator.naturalOrder(), Ordering.arbitrary()); + + // Get the number of cores on each node and sort the nodes by increasing number of cores + for (Node node : cluster.getLiveNodes()) { + // TODO: redo this. It is potentially less efficient to call propertyFetcher.getProperties() multiple times rather than once + final PropertyKey coresCountPropertyKey = propertyFactory.createCoreCountKey(node); + Map<PropertyKey, PropertyValue> propMap = propertyFetcher.fetchProperties(Collections.singleton(coresCountPropertyKey)); + PropertyValue returnedValue = propMap.get(coresCountPropertyKey); + if (returnedValue == null) { + throw new PlacementException("Can't get number of cores in " + node); Review comment: This doesn't have to be a fatal error - in some edge-cases the node could have gone down between the calls to `getLiveNodes` and `fetchProperties`, it could just mean that it's not eligible for placement. ########## File path: solr/core/src/java/org/apache/solr/cluster/placement/PropertyKeyFactory.java ########## @@ -0,0 +1,61 @@ +/* + * 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.cluster.placement; + +/** + * Factory used by the plugin to create property keys to request property values from Solr.<p> + * + * Building of a {@link PropertyKey} requires specifying the target (context) from which the value of that key should be + * obtained. This is done by specifying the appropriate {@link PropertyValueSource}.<br> + * For clarity, when only a single type of target is acceptable, the corresponding subtype of {@link PropertyValueSource} is used instead + * (for example {@link Node}). + */ +public interface PropertyKeyFactory { + /** + * Returns a property key to request the number of cores on a {@link Node}. + */ + PropertyKey createCoreCountKey(Node node); + + /** + * Returns a property key to request disk related info on a {@link Node}. + */ + PropertyKey createDiskInfoKey(Node node); + + /** + * Returns a property key to request the value of a system property on a {@link Node}. + * @param systemPropertyName the name of the system property to retrieve. + */ + PropertyKey createSystemPropertyKey(Node node, String systemPropertyName); + + /** + * Returns a property key to request the value of a metric.<p> + * + * Not all metrics make sense everywhere, but metrics can be applied to different objects. For example + * <code>SEARCHER.searcher.indexCommitSize</code> would make sense for a given replica of a given shard of a given collection, + * and possibly in other contexts.<p> + * + * @param metricSource The registry of the metric. For example a specific {@link Replica}. + * @param metricName for example <code>SEARCHER.searcher.indexCommitSize</code>. + */ + PropertyKey createMetricKey(PropertyValueSource metricSource, String metricName); Review comment: One node usually hosts many replicas. Each of these replicas has a unique registry name, in the form of `solr.core.<replicaName>`, so we could build PropertyKey from Replica because all components of the full metrics name are known. This is not the case with `node`, `jvm` and `jetty` - I think we need to explicitly specify the registry name in these cases. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
