sigram commented on a change in pull request #1684:
URL: https://github.com/apache/lucene-solr/pull/1684#discussion_r466617275
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -569,14 +574,20 @@ public AssignStrategy create(ClusterState clusterState,
DocCollection collection
case RULES:
List<Rule> rules = new ArrayList<>();
for (Object map : ruleMaps) rules.add(new Rule((Map) map));
+ @SuppressWarnings({"rawtypes"})
+ List snitches = (List) collection.get(SNITCH);
return new RulesBasedAssignStrategy(rules, snitches, clusterState);
+ case PLUGIN_PLACEMENT:
+ // TODO need to decide which plugin class to use. Global config
(single plugin for all PLUGIN_PLACEMENT collections?) or per collection config?
+ // TODO hardconding a sample plugin for now. DO NOT MERGE this as is.
+ return new PlacementPluginAssignStrategy(new
SamplePluginMinimizeCores());
default:
throw new Assign.AssignmentException("Unknown strategy type: " +
strategy);
}
}
private enum Strategy {
- LEGACY, RULES;
+ LEGACY, RULES, PLUGIN_PLACEMENT;
Review comment:
`Strategy` already describes how to perform placement. Maybe we should
rename `Strategy` -> `Placement` and simply use `PLUGIN` here?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/Cluster.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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 could then be done).
Review comment:
`no useful` -> `no useful work`
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/SystemPropertyPropertyValue.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 {@link PropertyValue} representing a System property on the target {@link
Node}.
+ */
+public interface SystemPropertyPropertyValue extends PropertyValue {
Review comment:
Maybe rename it to `SyspropPropertyValue` to avoid this weird repetition?
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/MetricPropertyValue.java
##########
@@ -0,0 +1,30 @@
+/*
+ * 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 {@link PropertyValue} representing a metric on the target {@link
PropertyValueSource}.
+ * Note there might be overlap with {@link SystemLoadPropertyValue} (only
applicable to {@link Node}'s), may need to clarify.
+ */
+public interface MetricPropertyValue extends PropertyValue {
+ /**
+ * Returns the metric value from the {@link PropertyValueSource} on which it
was retrieved.
+ * TODO: what type should the metric be? Maybe offer multiple getters for
different java types and have each metric implement the right one and throw
from the wrong ones? This avoids casting...
Review comment:
Practically we can encounter just three general types here:
* `NumberMetricPropertyValue` for numeric values
* `StringMetricPropertyValue` for plain strings
* `MapMetricPropertyValue` for some types of pseudo-metrics (gauges such as
eg. system properties), or fully detailed representation of many other metrics
(eg. histograms, meters, timers, etc).
##########
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);
Review comment:
IMHO this model is very limiting .. because every time we add another
type of `PropertyKey` we have to change this interface.
How about something like this?
`
<T> T createPropertyKey(Node node, Class<T extends PropertyKey>
propertyClass, String... props)
`
(or something like that ... not sure about the generics syntax here)
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/AddReplicasRequest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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;
+
+/**
+ * <p>Request for creating one or more {@link Replica}'s for one or more
{@link Shard}'s of an existing {@link SolrCollection}.
+ * The shard might or might not already exist, plugin code can easily find out
by using {@link SolrCollection#getShards()}
+ * and verifying if the shard name(s) from {@link #getShardNames()} are there.
+ *
+ * <p>As opposed to {@link CreateNewCollectionRequest}, the set of {@link
Node}s on which the replicas should be placed
+ * is specified (defaults to being equal to the set returned by {@link
Cluster#getLiveNodes()}).
+ *
+ * <p>There is no extension between this interface and {@link
CreateNewCollectionRequest} in either direction
+ * or from a common ancestor for readability. An ancestor could make sense and
would be an "abstract interface" not intended
+ * to be implemented directly, but this does not exist in Java.
+ *
+ * <p>Plugin code would likely treat the two types of requests differently
since here existing {@link Replica}'s must be taken
+ * into account for placement whereas in {@link CreateNewCollectionRequest} no
{@link Replica}'s are assumed to exist.
+ */
+public interface AddReplicasRequest extends Request {
+ /**
+ * The {@link SolrCollection} to add {@link Replica}(s) to. The replicas are
to be added to a shard that might or might
+ * not yet exist when the plugin's {@link PlacementPlugin#computePlacement}
is called.
+ */
+ SolrCollection getCollection();
+
+ /**
+ * <p>Shard name(s) for which new replicas placement should be computed. The
shard(s) might exist or not (that's why this
+ * method returns a {@link Set} of {@link String}'s and not directly a set
of {@link Shard} instances).
+ *
+ * <p>Note the Collection API allows specifying the shard name or a {@code
_route_} parameter. The Solr implementation will
+ * convert either specification into the relevant shard name so the plugin
code doesn't have to worry about this.
+ */
+ Set<String> getShardNames();
+
+ /** Replicas should only be placed on nodes from the set returned by this
method. */
+ Set<Node> getTargetNodes();
Review comment:
Or null / empty to mean all live nodes?
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/CoresCountPropertyValue.java
##########
@@ -0,0 +1,26 @@
+/*
+ * 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;
+
+public interface CoresCountPropertyValue extends PropertyValue {
+ /**
+ * Returns the number of cores on the {@link Node}) this instance was
obtained from (i.e. instance
+ * passed to {@link PropertyKeyFactory#createCoreCountKey(Node)}).
+ */
+ int getCoresCount();
Review comment:
Later we may consider extending this to report loaded / transient / lazy
cores.
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/ReplicaPlacement.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+/**
+ * <p>Placement decision for a single {@link Replica}. Note this placement
decision is used as part of a {@link WorkOrder},
+ * it does not directly lead to the plugin code getting a corresponding {@link
Replica} instance, nor does it require the
+ * plugin to provide a {@link Shard} instance (the plugin code gets such
instances for existing replicas and shards in the
+ * cluster but does not create them directly for adding new replicas for new
or existing shards).
+ *
+ * <p>Captures the {@link Shard} (via the shard name), {@link Node} and {@link
Replica.ReplicaType} of a Replica to be created.
+ */
+public interface ReplicaPlacement {
Review comment:
I think we must be more explicit here - the placement refers to Request
(very important for debugging and audit of the plugin decisions!), and it
should at the very least include the target node.
##########
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:
We have two sets of "global" metrics for a Solr instance: JVM and
node-level. I understand we could use Node to select node-level metrics (from
`solr.node` registry) but how to select `solr.jvm` metrics? Perhaps we need a
`JVMPropertyValueSource`
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/Cluster.java
##########
@@ -0,0 +1,46 @@
+/*
+ * 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 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;
Review comment:
I think we're missing a method to actually list all collection names.
Otherwise the plugins won't be able to make decisions based on more than 1
collection.
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/CreateNewCollectionRequest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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;
+
+/**
+ * <p>Request for creating a new collection with a given set of shards and
replication factor for various replica types.
+ * The expected {@link WorkOrder} corresponding to this {@link Request} is
created using
+ * {@link WorkOrderFactory#createWorkOrderNewCollection}
+ *
+ * <p>Note there is no need at this stage to allow the plugin to know each
shard hash range for example, this can be handled
+ * by the Solr side implementation of this interface without needing the
plugin to worry about it (the implementation of this interface on
+ * the Solr side can maintain the ranges for each shard).
+ *
+ * <p>Same goes for the {@link org.apache.solr.core.ConfigSet} name or other
collection parameters. They are needed for
+ * creating a Collection but likely do not have to be exposed to the plugin
(this can easily be changed if needed by
+ * adding accessors here, the underlying Solr side implementation of this
interface has the information).
+ */
+public interface CreateNewCollectionRequest extends Request {
+ /**
+ * <p>The name of the collection to be created and for which placement
should be computed.
+ *
+ * <p>Compare this method with {@link AddReplicasRequest#getCollection()},
there the collection already exists so can be
+ * directly passed in the {@link Request}.
+ *
+ * <p>When processing this request, plugin code doesn't have to worry about
existing {@link Replica}'s for the collection
+ * given that the collection is assumed not to exist.
+ */
+ String getCollectionName();
+
+ Set<String> getShardNames();
+
+ /**
+ * <p>Properties passed through the Collection API by the client creating
the collection.
+ * See {@link SolrCollection#getCustomProperty(String)}.
+ *
+ * <p>Given this {@link Request} is for creating a new collection, it is not
possible to pass the custom property values through
+ * the {@link SolrCollection} object. That instance does not exist yet, and
is the reason {@link #getCollectionName()} exists
+ * rather than a method returning {@link SolrCollection}...
+ */
+ String getCustomProperty(String customPropertyName);
Review comment:
I'm missing a method to enumerate the available properties, to be able
to pass them on to the CREATE command. Solr side can be responsible for acting
on these props but it needs to know that they exist - and checking for
existence of every possible property seems too tedious.
##########
File path:
solr/core/src/java/org/apache/solr/cluster/placement/PlacementException.java
##########
@@ -0,0 +1,48 @@
+/*
+ * 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;
+
+/**
+ * Exception thrown by a {@link PlacementPlugin} when it is unable to compute
placement for whatever reason (except an
Review comment:
We could make it immediately more useful by providing a required
`reason` arg, either an enum (easier to check but hard to extend) or one of the
predefined String constants.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/PropertyKey.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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 property key used by plugins to request values from Solr
+ */
+public interface PropertyKey {
+ /**
+ * @return the target of this {@link PropertyKey}, i.e. from where the
corresponding {@link PropertyValue}'s should be
+ * (or were) obtained.
+ */
+ PropertyValueSource getPropertyKeyTarget();
Review comment:
Uh ... why not use `getPropertyValueSource` or `getTarget` ? mixing the
"key", "value", "source" and "target" in one method is very confusing. Javadoc
can explain what the method actually does.
----------------------------------------------------------------
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
