You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/01/13 11:51:42 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
sigram opened a new pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199
See Jira for details.
This is a minimal approach that helps to set-up the initial co-location and then may veto collection layout changes (additions / removals or replicas) if it would violate the `withCollection` constraint.
This PR also refactors the placement API to add support for other types of "collection modification" requests, and extends the placement plugin API to add a method for vetoing such changes. `DeleteReplicaCmd` is also modified to make use of this functionality.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560099853
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java
##########
@@ -92,6 +98,19 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
collection = extCollection;
}
+ PlacementPlugin placementPlugin = ocmh.overseer.getCoreContainer().getPlacementPluginFactory().createPluginInstance();
Review comment:
The check could be simplified in the plugin if it only knew that we're deleting the whole collection. I guess we could create a `DeleteCollectionRequest` to let it know the type of modification we want to do.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] chatman commented on pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#issuecomment-763606464
Even a "minimal approach" approach seems to touch 30 files, many of them pertaining to collection API commands. I'm really scared of the complexity here. Can you please consider dropping "withCollection" feature altogether? I've added a comment in JIRA to that effect.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559751155
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/DeleteReplicasRequest.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;
+
+import org.apache.solr.cluster.Replica;
+
+import java.util.Set;
+
+/**
+ *
Review comment:
Javadoc needed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559767267
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +247,87 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (!(modificationRequest instanceof DeleteReplicasRequest)) {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ DeleteReplicasRequest request = (DeleteReplicasRequest) modificationRequest;
+ SolrCollection secondaryCollection = request.getCollection();
+ if (!withCollections.values().contains(secondaryCollection.getName())) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Cluster cluster = placementContext.getCluster();
+ Set<SolrCollection> colocatedCollections = new HashSet<>();
+ AtomicReference<IOException> exc = new AtomicReference<>();
Review comment:
This variable and how it's handled in the lambda below (and after the lambda) it too complex. If the `forEach` is replaced by a loop (a foreach loop...) the code is a lot simpler (and I believe shorter, although I didn't try to write it).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560098602
##########
File path: solr/core/src/java/org/apache/solr/cloud/ExclusiveSliceProperty.java
##########
@@ -74,8 +74,8 @@
ExclusiveSliceProperty(ClusterState clusterState, ZkNodeProps message) {
this.clusterState = clusterState;
String tmp = message.getStr(ZkStateReader.PROPERTY_PROP);
- if (StringUtils.startsWith(tmp, OverseerCollectionMessageHandler.COLL_PROP_PREFIX) == false) {
- tmp = OverseerCollectionMessageHandler.COLL_PROP_PREFIX + tmp;
+ if (StringUtils.startsWith(tmp, CollectionAdminParams.PROPERTY_PREFIX) == false) {
Review comment:
Heh, you touched it - you own it :)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559742269
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java
##########
@@ -92,6 +98,19 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
collection = extCollection;
}
+ PlacementPlugin placementPlugin = ocmh.overseer.getCoreContainer().getPlacementPluginFactory().createPluginInstance();
Review comment:
Didn't dig into the details here, but when we delete a collection, we should just check if there's another collection that defines `withCollection` on it and refuse the delete based on that, no?
Just starting to look at the PR so maybe there's a reason for doing it this way (in which case maybe adding a comment?)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram edited a comment on pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram edited a comment on pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#issuecomment-764692388
> Even a "minimal approach" approach seems to touch 30 files
@chatman Please see my response in Jira - some of the changed files are caused by moving a property, other are related to extending the plugin API to allow any kind of veto, not just `withCollection`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559751454
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/DeleteShardsRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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;
+
+/**
+ *
Review comment:
Javadoc needed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559750891
##########
File path: solr/core/src/java/org/apache/solr/cloud/overseer/ReplicaMutator.java
##########
@@ -115,8 +116,8 @@ public ZkWriteCommand addReplicaProperty(ClusterState clusterState, ZkNodeProps
String sliceName = message.getStr(ZkStateReader.SHARD_ID_PROP);
String replicaName = message.getStr(ZkStateReader.REPLICA_PROP);
String property = message.getStr(ZkStateReader.PROPERTY_PROP).toLowerCase(Locale.ROOT);
- if (StringUtils.startsWith(property, OverseerCollectionMessageHandler.COLL_PROP_PREFIX) == false) {
- property = OverseerCollectionMessageHandler.COLL_PROP_PREFIX + property;
+ if (StringUtils.startsWith(property, CollectionAdminParams.PROPERTY_PREFIX) == false) {
Review comment:
`== false` -> `!`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram closed pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559740603
##########
File path: solr/core/src/java/org/apache/solr/cloud/ExclusiveSliceProperty.java
##########
@@ -74,8 +74,8 @@
ExclusiveSliceProperty(ClusterState clusterState, ZkNodeProps message) {
this.clusterState = clusterState;
String tmp = message.getStr(ZkStateReader.PROPERTY_PROP);
- if (StringUtils.startsWith(tmp, OverseerCollectionMessageHandler.COLL_PROP_PREFIX) == false) {
- tmp = OverseerCollectionMessageHandler.COLL_PROP_PREFIX + tmp;
+ if (StringUtils.startsWith(tmp, CollectionAdminParams.PROPERTY_PREFIX) == false) {
Review comment:
Minor: use `!` rather than `== false` (I know it's old code but you touched it :)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559759523
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ * @return
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ Shard shard = solrCollection.getShard(shardName);
+ Slice slice = docCollection.getSlice(shardName);
+ Set<Replica> solrReplicas = new HashSet<>();
+ replicaNames.forEach(name -> {
+ org.apache.solr.common.cloud.Replica replica = slice.getReplica(name);
+ Replica solrReplica = new SimpleClusterAbstractionsImpl.ReplicaImpl(replica.getName(), shard, replica);
Review comment:
Why not just do `solrReplicas.add(shard.getReplica(name))` in the `forEach`?
Or an easier to read IMO (personal preference but a lambda here is fine):
`for (String name : replicaNames) { solrReplicas.add(shard.getReplica(name)); }`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559761033
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/PlacementContext.java
##########
@@ -0,0 +1,44 @@
+/*
+ * 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 org.apache.solr.cluster.Cluster;
+
+/**
+ *
Review comment:
Javadoc
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementContextImpl.java
##########
@@ -0,0 +1,39 @@
+package org.apache.solr.cluster.placement.impl;
+
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
+import org.apache.solr.cluster.Cluster;
+import org.apache.solr.cluster.placement.AttributeFetcher;
+import org.apache.solr.cluster.placement.PlacementContext;
+import org.apache.solr.cluster.placement.PlacementPlanFactory;
+
+import java.io.IOException;
+
+/**
+ *
+ */
+public class PlacementContextImpl implements PlacementContext {
Review comment:
Maybe have "Simple" somewhere in the name of this class given it's instantiating `SimpleClusterAbstractionsImpl`?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ * @return
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ Shard shard = solrCollection.getShard(shardName);
+ Slice slice = docCollection.getSlice(shardName);
+ Set<Replica> solrReplicas = new HashSet<>();
+ replicaNames.forEach(name -> {
+ org.apache.solr.common.cloud.Replica replica = slice.getReplica(name);
+ Replica solrReplica = new SimpleClusterAbstractionsImpl.ReplicaImpl(replica.getName(), shard, replica);
Review comment:
Or do it with streams...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559749942
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteReplicaCmd.java
##########
@@ -147,14 +170,27 @@ void deleteReplicaBasedOnCount(ClusterState clusterState,
}
}
+ if (placementPlugin != null) {
Review comment:
By adding `if (placementPlugin != null)` logic in the `*Cmd` classes, we are breaking the encapsulation that placement logic is handled by `Assign.AssignStrategy`.
The only reason `*Cmd` code currently (before this PR) is even aware of the notion of `PlacementPlugin` is because `PlacementPluginAssignStrategy` configuration (rather than `LegacyAssignStrategy`) is dependent on a plugin being defined...
I suggest to move all `*Cmd` vetting logic to `Assign.AssignStrategy`, so that `*Cmd` only need to pass the instance of `PlacementPlugin` to that code (and later when we finally decide how clusters are configured this will go away, and we keep `*Cmd` clean of any specific assign strategy behavior).
In `Assign.AssignStrategy` the default vetting logic will be "accept all" and in `PlacementPluginAssignStrategy` we can implement the checks we like.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] muse-dev[bot] commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r556520136
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ *
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+ };
+ }
+
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ Shard shard = solrCollection.getShard(shardName);
Review comment:
*NULL_DEREFERENCE:* object `solrCollection` last assigned on line 51 could be null and is dereferenced at line 52.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559762633
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java
##########
@@ -324,7 +324,7 @@ public int hashCode() {
return new Pair<>(replicas, leader);
}
- private ReplicaImpl(String replicaName, Shard shard, org.apache.solr.common.cloud.Replica sliceReplica) {
+ ReplicaImpl(String replicaName, Shard shard, org.apache.solr.common.cloud.Replica sliceReplica) {
Review comment:
Why are these no longer `private`?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#issuecomment-764692388
> Even a "minimal approach" approach seems to touch 30 files
Please see my response in Jira - some of the changed files are caused by moving a property, other are related to extending the plugin API to allow any kind of veto, not just `withCollection`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r564405099
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
I don't think we should complicate the implementation to handle this. If the user created a cycle then deleting any collection in the cycle will be impossible - but this can be easily remedied by modifying the configuration of the plugin (which takes effect nearly immediately) to break the cycle.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r563645684
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
Can we have cycles in the `withCollection` graph? Should we allow a way to override the vetting checks from the Collection API?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/DeleteShardsRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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;
+
+/**
+ * Delete shards request.
+ */
+public interface DeleteShardsRequest extends ModificationRequest {
Review comment:
If we don't use this interface (i.e. the class that implements it) I suggest we do not include either in this PR. Or at least define and call the corresponding method in `AssignStrategy` from the appropriate `*Cmd` even if nothing does a real implementation and vetting based on it (but it would be ready to be consumed maybe by another plugin written by some user).
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
Review comment:
here too. Allow everything we don't plan to explicitly disallow.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -467,7 +569,7 @@ private void makePlacementDecisions(SolrCollection solrCollection, String shardN
if (candidateAzEntries == null) {
// This can happen because not enough nodes for the placement request or already too many nodes with replicas of
// the shard that can't accept new replicas or not enough nodes with enough free disk space.
- throw new PlacementException("Not enough nodes to place " + numReplicas + " replica(s) of type " + replicaType +
+ throw new PlacementException("Not enough eligible nodes to place " + numReplicas + " replica(s) of type " + replicaType +
Review comment:
Not sure if it's useful here or output elsewhere already though.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java
##########
@@ -43,14 +46,28 @@
@JsonProperty
public long prioritizedFreeDiskGB;
+ /**
+ * This property defines an additional constraint that primary collections (keys) should be
+ * located on the same nodes as the secondary collections (values). The plugin will assume
+ * that the secondary collection replicas are already in place and ignore candidate nodes where
+ * they are not already present.
+ */
+ @JsonProperty
+ public Map<String, String> withCollections;
+
// no-arg public constructor required for deserialization
public AffinityPlacementConfig() {
- minimalFreeDiskGB = 20L;
- prioritizedFreeDiskGB = 100L;
+ this (20L, 100L, Map.of());
Review comment:
Also, this constructor should call the constructor not requiring a `Map`, it will add an empty one.
BTW, the deserialization constructor should not pass "meaningful" default values since those will not be used.
I'd call `this(0L, 0L)` here instead, and switch the `AffinityPlacementConfig DEFAULT` static variable to use the constructor accepting values. That will also make seeing what are the defaults easier when reading the code.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -467,7 +569,7 @@ private void makePlacementDecisions(SolrCollection solrCollection, String shardN
if (candidateAzEntries == null) {
// This can happen because not enough nodes for the placement request or already too many nodes with replicas of
// the shard that can't accept new replicas or not enough nodes with enough free disk space.
- throw new PlacementException("Not enough nodes to place " + numReplicas + " replica(s) of type " + replicaType +
+ throw new PlacementException("Not enough eligible nodes to place " + numReplicas + " replica(s) of type " + replicaType +
Review comment:
Can we add in the message if there were `withCollection` constraints used in the placement logic? No details, not even indicating that it's the failure reason, just mentioning it...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
Review comment:
If not implemented we should let it happen rather than have it fail.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
+ }
+ } catch (IOException e) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ }
+ }
+ }
+
+ private void verifyDeleteReplicas(DeleteReplicasRequest deleteReplicasRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ SolrCollection secondaryCollection = deleteReplicasRequest.getCollection();
+ Set<String> colocatedCollections = colocatedWith.get(secondaryCollection.getName());
+ if (colocatedCollections == null) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Map<Node, Set<String>> colocatingNodes = new HashMap<>();
+ try {
+ for (String colocatedCollection : colocatedCollections) {
+ SolrCollection coll = cluster.getCollection(colocatedCollection);
+ coll.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ colocatingNodes.computeIfAbsent(replica.getNode(), n -> new HashSet<>())
+ .add(coll.getName());
+ }));
+ }
+ } catch (IOException ioe) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", ioe);
+ }
+ PlacementModificationException exception = null;
+ for (Replica replica : deleteReplicasRequest.getReplicas()) {
+ if (!colocatingNodes.containsKey(replica.getNode())) {
+ continue;
+ }
+ // check that there will be at least one replica remaining
+ AtomicInteger secondaryCount = secondaryNodeShardReplicas
+ .getOrDefault(replica.getNode(), Map.of())
+ .getOrDefault(replica.getShard().getShardName(), new AtomicInteger());
+ if (secondaryCount.get() > 1) {
+ // we can delete it - record the deletion
+ secondaryCount.decrementAndGet();
+ continue;
+ }
+ // fail - this replica cannot be removed
+ if (exception == null) {
+ exception = new PlacementModificationException("delete replica(s) rejected");
+ }
+ exception.addRejectedModification(replica.toString(), "co-located with replicas of " + colocatingNodes.get(replica.getNode()));
Review comment:
Add the problematic node in the rejection reason so that client can delete the colocated collection replicas there to fix the issue.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
Review comment:
The exception should also tell which collection we refuse to delete (unless that context will be added higher up the call stack?).
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
Review comment:
Method should be called `createDeleteReplicasRequest`?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java
##########
@@ -43,14 +46,28 @@
@JsonProperty
public long prioritizedFreeDiskGB;
+ /**
+ * This property defines an additional constraint that primary collections (keys) should be
+ * located on the same nodes as the secondary collections (values). The plugin will assume
+ * that the secondary collection replicas are already in place and ignore candidate nodes where
+ * they are not already present.
+ */
+ @JsonProperty
+ public Map<String, String> withCollections;
+
// no-arg public constructor required for deserialization
public AffinityPlacementConfig() {
- minimalFreeDiskGB = 20L;
- prioritizedFreeDiskGB = 100L;
+ this (20L, 100L, Map.of());
Review comment:
minor: remove space after `this`.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
Review comment:
The signature is not consistent with `deleteReplicasRequest` above that takes `Replica` instances rather than `String` names. I prefer the strongly typed signature.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -381,7 +382,11 @@ public AssignmentException(String message, Throwable cause, boolean enableSuppre
public interface AssignStrategy {
List<ReplicaPosition> assign(SolrCloudManager solrCloudManager, AssignRequest assignRequest)
- throws Assign.AssignmentException, IOException, InterruptedException;
+ throws AssignmentException, IOException, InterruptedException;
+ void verifyDeleteCollection(SolrCloudManager solrCloudManager, DocCollection collection)
Review comment:
Maybe add default implementation here to not force empty implementation in legacy or other places?
I'd add javadoc at least for added methods...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
+ }
+ } catch (IOException e) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ }
+ }
+ }
+
+ private void verifyDeleteReplicas(DeleteReplicasRequest deleteReplicasRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ SolrCollection secondaryCollection = deleteReplicasRequest.getCollection();
+ Set<String> colocatedCollections = colocatedWith.get(secondaryCollection.getName());
+ if (colocatedCollections == null) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Map<Node, Set<String>> colocatingNodes = new HashMap<>();
+ try {
+ for (String colocatedCollection : colocatedCollections) {
+ SolrCollection coll = cluster.getCollection(colocatedCollection);
+ coll.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ colocatingNodes.computeIfAbsent(replica.getNode(), n -> new HashSet<>())
+ .add(coll.getName());
+ }));
+ }
+ } catch (IOException ioe) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", ioe);
+ }
+ PlacementModificationException exception = null;
+ for (Replica replica : deleteReplicasRequest.getReplicas()) {
+ if (!colocatingNodes.containsKey(replica.getNode())) {
+ continue;
+ }
+ // check that there will be at least one replica remaining
+ AtomicInteger secondaryCount = secondaryNodeShardReplicas
+ .getOrDefault(replica.getNode(), Map.of())
+ .getOrDefault(replica.getShard().getShardName(), new AtomicInteger());
+ if (secondaryCount.get() > 1) {
+ // we can delete it - record the deletion
+ secondaryCount.decrementAndGet();
+ continue;
+ }
+ // fail - this replica cannot be removed
+ if (exception == null) {
+ exception = new PlacementModificationException("delete replica(s) rejected");
+ }
+ exception.addRejectedModification(replica.toString(), "co-located with replicas of " + colocatingNodes.get(replica.getNode()));
Review comment:
We do not support in Solr returning errors from Collection API calls in a way that is consumable by code, right? Would be a nice addition...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ Shard shard = solrCollection.getShard(shardName);
+ Slice slice = docCollection.getSlice(shardName);
+ Set<Replica> solrReplicas = new HashSet<>();
+ replicaNames.forEach(name -> solrReplicas.add(shard.getReplica(name)));
+ return deleteReplicasRequest(solrCollection, solrReplicas);
+ }
+
+
+ public static DeleteShardsRequest deleteShardsRequest(SolrCollection collection, Set<String> shardNames) {
Review comment:
This and next method should really be called `createDeleteShardsRequest` to convey what they really do, right?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559768110
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +247,87 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
This method should IMO be factored out and cut into a few pieces with meaningful names to make reading easier.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560111293
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -171,14 +174,17 @@ public AffinityPlacementConfig getConfig() {
private final long prioritizedFreeDiskGB;
+ private final Map<String, String> withCollections;
Review comment:
There's no reason not to extend this to N:1 (N primary collections with 1 secondary), I just followed the model that was present in 8x.
However, the way this co-location is implemented now may degrade the placements if we allow N:1 or N:N models, because of the pre-filtering approach we could quickly run out of candidate nodes for placements. So I prefer to focus on implementing 1:1 now and making sure it works well.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r564413143
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
I agree. Or change the configuration of the collection (I assume `withCollection` can be removed).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559766377
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -171,14 +174,17 @@ public AffinityPlacementConfig getConfig() {
private final long prioritizedFreeDiskGB;
+ private final Map<String, String> withCollections;
Review comment:
Q: a given collection can only be `withCollection` for a single secondary collection? Doesn't seem necessary...
Suggestion: maintain the inverse mapping as well (a multimap, but possibly this one should be a multimap as well) to save looping through map keys checking values...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#issuecomment-764692388
> Even a "minimal approach" approach seems to touch 30 files
Please see my response in Jira - some of the changed files are caused by moving a property, other are related to extending the plugin API to allow any kind of veto, not just `withCollection`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559766377
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -171,14 +174,17 @@ public AffinityPlacementConfig getConfig() {
private final long prioritizedFreeDiskGB;
+ private final Map<String, String> withCollections;
Review comment:
Q: a given collection can only be `withCollection` for a single secondary collection? Doesn't seem to be a necessary limitation...
Suggestion: maintain the inverse mapping as well (a multimap, but possibly this one should be a multimap as well) to save looping through map keys checking values...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559768110
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +247,87 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
This method should be factored out and cut into a few pieces with meaningful names to make reading easier.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r564426476
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -467,7 +569,7 @@ private void makePlacementDecisions(SolrCollection solrCollection, String shardN
if (candidateAzEntries == null) {
// This can happen because not enough nodes for the placement request or already too many nodes with replicas of
// the shard that can't accept new replicas or not enough nodes with enough free disk space.
- throw new PlacementException("Not enough nodes to place " + numReplicas + " replica(s) of type " + replicaType +
+ throw new PlacementException("Not enough eligible nodes to place " + numReplicas + " replica(s) of type " + replicaType +
Review comment:
it's already handled in `filterNodesWithCollection`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559763883
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java
##########
@@ -43,14 +46,30 @@
@JsonProperty
public long prioritizedFreeDiskGB;
+ /**
+ * This property defines an additional constraint that primary collections (keys) should be
+ * located on the same nodes as the secondary collections (values). The plugin will assume
+ * that the secondary collection replicas are already in place and ignore candidate nodes where
+ * they are not already present.
+ */
+ @JsonProperty
+ public Map<String, String> withCollections;
+
// no-arg public constructor required for deserialization
public AffinityPlacementConfig() {
minimalFreeDiskGB = 20L;
Review comment:
I prefer the no arg constructor here to call the appropriate constructor. That way logic is not replicated (might not be applicable here unless somebody replaces `withCollections = Map.of();` with `withCollections = null;` in a future commit), it looks cleaner and by tracing calls to the most complete constructor all callers are inventoried...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559753902
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/DeleteShardsRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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;
+
+/**
+ *
Review comment:
Also, this interface is implemented but the implementation is never used.
Unless we implement a use for it in this PR, I suggest we leave it out until we actually need it. I assume we don't need it for `withCollection` because the secondary collection has to be single shard so that shard will not be deleted.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r563645684
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
Review comment:
Can we have cycles in the `withCollection` graph? Should we allow a way to override the vetting checks from the Collection API?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/DeleteShardsRequest.java
##########
@@ -0,0 +1,27 @@
+/*
+ * 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;
+
+/**
+ * Delete shards request.
+ */
+public interface DeleteShardsRequest extends ModificationRequest {
Review comment:
If we don't use this interface (i.e. the class that implements it) I suggest we do not include either in this PR. Or at least define and call the corresponding method in `AssignStrategy` from the appropriate `*Cmd` even if nothing does a real implementation and vetting based on it (but it would be ready to be consumed maybe by another plugin written by some user).
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
Review comment:
here too. Allow everything we don't plan to explicitly disallow.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -467,7 +569,7 @@ private void makePlacementDecisions(SolrCollection solrCollection, String shardN
if (candidateAzEntries == null) {
// This can happen because not enough nodes for the placement request or already too many nodes with replicas of
// the shard that can't accept new replicas or not enough nodes with enough free disk space.
- throw new PlacementException("Not enough nodes to place " + numReplicas + " replica(s) of type " + replicaType +
+ throw new PlacementException("Not enough eligible nodes to place " + numReplicas + " replica(s) of type " + replicaType +
Review comment:
Not sure if it's useful here or output elsewhere already though.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java
##########
@@ -43,14 +46,28 @@
@JsonProperty
public long prioritizedFreeDiskGB;
+ /**
+ * This property defines an additional constraint that primary collections (keys) should be
+ * located on the same nodes as the secondary collections (values). The plugin will assume
+ * that the secondary collection replicas are already in place and ignore candidate nodes where
+ * they are not already present.
+ */
+ @JsonProperty
+ public Map<String, String> withCollections;
+
// no-arg public constructor required for deserialization
public AffinityPlacementConfig() {
- minimalFreeDiskGB = 20L;
- prioritizedFreeDiskGB = 100L;
+ this (20L, 100L, Map.of());
Review comment:
Also, this constructor should call the constructor not requiring a `Map`, it will add an empty one.
BTW, the deserialization constructor should not pass "meaningful" default values since those will not be used.
I'd call `this(0L, 0L)` here instead, and switch the `AffinityPlacementConfig DEFAULT` static variable to use the constructor accepting values. That will also make seeing what are the defaults easier when reading the code.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -467,7 +569,7 @@ private void makePlacementDecisions(SolrCollection solrCollection, String shardN
if (candidateAzEntries == null) {
// This can happen because not enough nodes for the placement request or already too many nodes with replicas of
// the shard that can't accept new replicas or not enough nodes with enough free disk space.
- throw new PlacementException("Not enough nodes to place " + numReplicas + " replica(s) of type " + replicaType +
+ throw new PlacementException("Not enough eligible nodes to place " + numReplicas + " replica(s) of type " + replicaType +
Review comment:
Can we add in the message if there were `withCollection` constraints used in the placement logic? No details, not even indicating that it's the failure reason, just mentioning it...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
Review comment:
If not implemented we should let it happen rather than have it fail.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
+ }
+ } catch (IOException e) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ }
+ }
+ }
+
+ private void verifyDeleteReplicas(DeleteReplicasRequest deleteReplicasRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ SolrCollection secondaryCollection = deleteReplicasRequest.getCollection();
+ Set<String> colocatedCollections = colocatedWith.get(secondaryCollection.getName());
+ if (colocatedCollections == null) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Map<Node, Set<String>> colocatingNodes = new HashMap<>();
+ try {
+ for (String colocatedCollection : colocatedCollections) {
+ SolrCollection coll = cluster.getCollection(colocatedCollection);
+ coll.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ colocatingNodes.computeIfAbsent(replica.getNode(), n -> new HashSet<>())
+ .add(coll.getName());
+ }));
+ }
+ } catch (IOException ioe) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", ioe);
+ }
+ PlacementModificationException exception = null;
+ for (Replica replica : deleteReplicasRequest.getReplicas()) {
+ if (!colocatingNodes.containsKey(replica.getNode())) {
+ continue;
+ }
+ // check that there will be at least one replica remaining
+ AtomicInteger secondaryCount = secondaryNodeShardReplicas
+ .getOrDefault(replica.getNode(), Map.of())
+ .getOrDefault(replica.getShard().getShardName(), new AtomicInteger());
+ if (secondaryCount.get() > 1) {
+ // we can delete it - record the deletion
+ secondaryCount.decrementAndGet();
+ continue;
+ }
+ // fail - this replica cannot be removed
+ if (exception == null) {
+ exception = new PlacementModificationException("delete replica(s) rejected");
+ }
+ exception.addRejectedModification(replica.toString(), "co-located with replicas of " + colocatingNodes.get(replica.getNode()));
Review comment:
Add the problematic node in the rejection reason so that client can delete the colocated collection replicas there to fix the issue.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
Review comment:
The exception should also tell which collection we refuse to delete (unless that context will be added higher up the call stack?).
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
Review comment:
Method should be called `createDeleteReplicasRequest`?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementConfig.java
##########
@@ -43,14 +46,28 @@
@JsonProperty
public long prioritizedFreeDiskGB;
+ /**
+ * This property defines an additional constraint that primary collections (keys) should be
+ * located on the same nodes as the secondary collections (values). The plugin will assume
+ * that the secondary collection replicas are already in place and ignore candidate nodes where
+ * they are not already present.
+ */
+ @JsonProperty
+ public Map<String, String> withCollections;
+
// no-arg public constructor required for deserialization
public AffinityPlacementConfig() {
- minimalFreeDiskGB = 20L;
- prioritizedFreeDiskGB = 100L;
+ this (20L, 100L, Map.of());
Review comment:
minor: remove space after `this`.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
Review comment:
The signature is not consistent with `deleteReplicasRequest` above that takes `Replica` instances rather than `String` names. I prefer the strongly typed signature.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -381,7 +382,11 @@ public AssignmentException(String message, Throwable cause, boolean enableSuppre
public interface AssignStrategy {
List<ReplicaPosition> assign(SolrCloudManager solrCloudManager, AssignRequest assignRequest)
- throws Assign.AssignmentException, IOException, InterruptedException;
+ throws AssignmentException, IOException, InterruptedException;
+ void verifyDeleteCollection(SolrCloudManager solrCloudManager, DocCollection collection)
Review comment:
Maybe add default implementation here to not force empty implementation in legacy or other places?
I'd add javadoc at least for added methods...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +258,93 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (modificationRequest instanceof DeleteCollectionRequest) {
+ verifyDeleteCollection((DeleteCollectionRequest) modificationRequest, placementContext);
+ } else if (modificationRequest instanceof DeleteReplicasRequest) {
+ verifyDeleteReplicas((DeleteReplicasRequest) modificationRequest, placementContext);
+ } else {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ }
+
+ private void verifyDeleteCollection(DeleteCollectionRequest deleteCollectionRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ Set<String> colocatedCollections = colocatedWith.getOrDefault(deleteCollectionRequest.getCollection().getName(), Set.of());
+ for (String primaryName : colocatedCollections) {
+ try {
+ if (cluster.getCollection(primaryName) != null) {
+ // still exists
+ throw new PlacementModificationException("colocated collection " + primaryName + " still present");
+ }
+ } catch (IOException e) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ }
+ }
+ }
+
+ private void verifyDeleteReplicas(DeleteReplicasRequest deleteReplicasRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ Cluster cluster = placementContext.getCluster();
+ SolrCollection secondaryCollection = deleteReplicasRequest.getCollection();
+ Set<String> colocatedCollections = colocatedWith.get(secondaryCollection.getName());
+ if (colocatedCollections == null) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Map<Node, Set<String>> colocatingNodes = new HashMap<>();
+ try {
+ for (String colocatedCollection : colocatedCollections) {
+ SolrCollection coll = cluster.getCollection(colocatedCollection);
+ coll.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ colocatingNodes.computeIfAbsent(replica.getNode(), n -> new HashSet<>())
+ .add(coll.getName());
+ }));
+ }
+ } catch (IOException ioe) {
+ throw new PlacementModificationException("failed to retrieve colocated collection information", ioe);
+ }
+ PlacementModificationException exception = null;
+ for (Replica replica : deleteReplicasRequest.getReplicas()) {
+ if (!colocatingNodes.containsKey(replica.getNode())) {
+ continue;
+ }
+ // check that there will be at least one replica remaining
+ AtomicInteger secondaryCount = secondaryNodeShardReplicas
+ .getOrDefault(replica.getNode(), Map.of())
+ .getOrDefault(replica.getShard().getShardName(), new AtomicInteger());
+ if (secondaryCount.get() > 1) {
+ // we can delete it - record the deletion
+ secondaryCount.decrementAndGet();
+ continue;
+ }
+ // fail - this replica cannot be removed
+ if (exception == null) {
+ exception = new PlacementModificationException("delete replica(s) rejected");
+ }
+ exception.addRejectedModification(replica.toString(), "co-located with replicas of " + colocatingNodes.get(replica.getNode()));
Review comment:
We do not support in Solr returning errors from Collection API calls in a way that is consumable by code, right? Would be a nice addition...
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/ModificationRequestImpl.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.impl;
+
+import org.apache.solr.cluster.Replica;
+import org.apache.solr.cluster.Shard;
+import org.apache.solr.cluster.SolrCollection;
+import org.apache.solr.cluster.placement.DeleteCollectionRequest;
+import org.apache.solr.cluster.placement.DeleteReplicasRequest;
+import org.apache.solr.cluster.placement.DeleteShardsRequest;
+import org.apache.solr.common.cloud.DocCollection;
+import org.apache.solr.common.cloud.Slice;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper class to create modification request instances.
+ */
+public class ModificationRequestImpl {
+
+ public static DeleteCollectionRequest deleteCollectionRequest(DocCollection docCollection) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ return () -> solrCollection;
+ }
+
+ /**
+ * Create a delete replicas request.
+ * @param collection collection to delete replicas from
+ * @param replicas replicas to delete
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(SolrCollection collection, Set<Replica> replicas) {
+ return new DeleteReplicasRequest() {
+ @Override
+ public Set<Replica> getReplicas() {
+ return replicas;
+ }
+
+ @Override
+ public SolrCollection getCollection() {
+ return collection;
+ }
+
+ @Override
+ public String toString() {
+ return "DeleteReplicasRequest{collection=" + collection.getName() +
+ ",replicas=" + replicas;
+ }
+ };
+ }
+
+ /**
+ * Create a delete replicas request using the internal Solr API.
+ * @param docCollection Solr collection
+ * @param shardName shard name
+ * @param replicaNames replica names (aka. core-node names)
+ */
+ public static DeleteReplicasRequest deleteReplicasRequest(DocCollection docCollection, String shardName, Set<String> replicaNames) {
+ SolrCollection solrCollection = SimpleClusterAbstractionsImpl.SolrCollectionImpl.fromDocCollection(docCollection);
+ Shard shard = solrCollection.getShard(shardName);
+ Slice slice = docCollection.getSlice(shardName);
+ Set<Replica> solrReplicas = new HashSet<>();
+ replicaNames.forEach(name -> solrReplicas.add(shard.getReplica(name)));
+ return deleteReplicasRequest(solrCollection, solrReplicas);
+ }
+
+
+ public static DeleteShardsRequest deleteShardsRequest(SolrCollection collection, Set<String> shardNames) {
Review comment:
This and next method should really be called `createDeleteShardsRequest` to convey what they really do, right?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560104669
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java
##########
@@ -324,7 +324,7 @@ public int hashCode() {
return new Pair<>(replicas, leader);
}
- private ReplicaImpl(String replicaName, Shard shard, org.apache.solr.common.cloud.Replica sliceReplica) {
+ ReplicaImpl(String replicaName, Shard shard, org.apache.solr.common.cloud.Replica sliceReplica) {
Review comment:
Left-over from earlier refactoring, will revert.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r559755408
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteReplicaCmd.java
##########
@@ -147,14 +170,27 @@ void deleteReplicaBasedOnCount(ClusterState clusterState,
}
}
+ if (placementPlugin != null) {
Review comment:
In `Assign.AssignStrategy` if we want to be exhaustive, we should for example reject shard splits for secondary that are targets of `withCollection` (given we refuse such targets to have more than one shard).
Not saying we should do it, but the vetting infra we put in place should allow logical extension to all these aspects (with minor impact on the commands).
Also, pushing all the logic to `Assign.AssignStrategy` and minimizing changes to `*Cmd` limits the impact of a regression (moving to `LegacyAssignStrategy` should be a workaround for most problems).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560099853
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteCollectionCmd.java
##########
@@ -92,6 +98,19 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
collection = extCollection;
}
+ PlacementPlugin placementPlugin = ocmh.overseer.getCoreContainer().getPlacementPluginFactory().createPluginInstance();
Review comment:
The check can be simplified in the plugin if it only knew that we're deleting the whole collection. I guess we could create a `DeleteCollectionRequest` to let it know the type of modification we want to do.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#discussion_r560928329
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -238,11 +247,87 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
// failure. Current code does fail if placement is impossible (constraint is at most one replica of a shard on any node).
for (Replica.ReplicaType replicaType : Replica.ReplicaType.values()) {
makePlacementDecisions(solrCollection, shardName, availabilityZones, replicaType, request.getCountReplicasToCreate(replicaType),
- attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementPlanFactory, replicaPlacements);
+ attrValues, replicaTypeToNodes, nodesWithReplicas, coresOnNodes, placementContext.getPlacementPlanFactory(), replicaPlacements);
}
}
- return placementPlanFactory.createPlacementPlan(request, replicaPlacements);
+ return placementContext.getPlacementPlanFactory().createPlacementPlan(request, replicaPlacements);
+ }
+
+ @Override
+ public void verifyAllowedModification(ModificationRequest modificationRequest, PlacementContext placementContext) throws PlacementModificationException, InterruptedException {
+ if (modificationRequest instanceof DeleteShardsRequest) {
+ throw new UnsupportedOperationException("not implemented yet");
+ } else if (!(modificationRequest instanceof DeleteReplicasRequest)) {
+ throw new UnsupportedOperationException("unsupported request type " + modificationRequest.getClass().getName());
+ }
+ DeleteReplicasRequest request = (DeleteReplicasRequest) modificationRequest;
+ SolrCollection secondaryCollection = request.getCollection();
+ if (!withCollections.values().contains(secondaryCollection.getName())) {
+ return;
+ }
+ Map<Node, Map<String, AtomicInteger>> secondaryNodeShardReplicas = new HashMap<>();
+ secondaryCollection.shards().forEach(shard ->
+ shard.replicas().forEach(replica -> {
+ secondaryNodeShardReplicas.computeIfAbsent(replica.getNode(), n -> new HashMap<>())
+ .computeIfAbsent(replica.getShard().getShardName(), s -> new AtomicInteger())
+ .incrementAndGet();
+ }));
+
+ // find the colocated-with collections
+ Cluster cluster = placementContext.getCluster();
+ Set<SolrCollection> colocatedCollections = new HashSet<>();
+ AtomicReference<IOException> exc = new AtomicReference<>();
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram edited a comment on pull request #2199: SOLR-15055 (Take 2) Re-implement 'withCollection'
Posted by GitBox <gi...@apache.org>.
sigram edited a comment on pull request #2199:
URL: https://github.com/apache/lucene-solr/pull/2199#issuecomment-764692388
> Even a "minimal approach" approach seems to touch 30 files
@chatman Please see my response in Jira - some of the changed files are caused by moving a property, other are related to extending the plugin API to allow any kind of veto, not just `withCollection`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org