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/05 19:47:36 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
sigram opened a new pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179
See Jira for details.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552527986
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -193,6 +195,22 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
Set<Node> nodes = request.getTargetNodes();
SolrCollection solrCollection = request.getCollection();
+ final SolrCollection secondaryCollection;
+ String withCollection = solrCollection.getCustomProperty(CollectionAdminParams.WITH_COLLECTION);
Review comment:
I think that we should probably completely disallow using aliases in this context... how would that work in practice? if you flip the alias that used to point to the secondary collection to some other collection then suddenly the co-location may be completely broken.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552198751
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
+ if (!clusterState.hasCollection(realWithCollection)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "The 'withCollection' does not exist: " + realWithCollection);
+ } else {
+ DocCollection collection = clusterState.getCollection(realWithCollection);
+ if (collection.getActiveSlices().size() > 1) {
Review comment:
Maybe test !=1 to make sure it really has at least one? Friendlier to fail with the `SolrException` here than below with `next()` throwing `NoSuchElementException`.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
+ if (!clusterState.hasCollection(realWithCollection)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "The 'withCollection' does not exist: " + realWithCollection);
+ } else {
+ DocCollection collection = clusterState.getCollection(realWithCollection);
+ if (collection.getActiveSlices().size() > 1) {
Review comment:
Maybe test `!=1` to make sure it really has at least one? Friendlier to fail with the `SolrException` here than below with `next()` throwing `NoSuchElementException`.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552606865
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -254,8 +280,27 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
}
shardRequestTracker.processResponses(results, shardHandler, false, null, Collections.emptySet());
+
+ if (withCollection != null) {
+ // process replica placements for the secondary collection, if any are needed
+ for (ReplicaPosition replicaPosition : replicaPositions) {
+ if (!replicaPosition.collection.equals(withCollection)) {
+ continue;
+ }
+ ZkNodeProps props = new ZkNodeProps(
+ Overseer.QUEUE_OPERATION, ADDREPLICA.toString(),
+ ZkStateReader.COLLECTION_PROP, replicaPosition.collection,
+ ZkStateReader.SHARD_ID_PROP, replicaPosition.shard,
+ "node", replicaPosition.node,
+ CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.TRUE.toString()); // set to true because we want `withCollection` to be ready after this collection is created
+ new AddReplicaCmd(ocmh).call(clusterState, props, results);
+ clusterState = zkStateReader.getClusterState(); // refresh
Review comment:
It's a copy-pasta from 8x... I wasn't sure if it's required so it's good to know it doesn't make sense and 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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552226534
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
Review comment:
Or is the collection created with the `withCollection` property that is assumed to be an actual collection?
Wouldn't this somehow defeats the purpose of an alias? What prevents having the collection `withCollection` property follow aliases? Follow always? Or not always? The current proposal follows always given it doesn't check `followAliases`. In which case maybe resolving the alias each time replicas are added to the collection makes sense? Let the placement plugin do the failure, it's checking the `withCollection` collection is appropriate anyway.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552208910
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -193,6 +195,22 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
Set<Node> nodes = request.getTargetNodes();
SolrCollection solrCollection = request.getCollection();
+ final SolrCollection secondaryCollection;
+ String withCollection = solrCollection.getCustomProperty(CollectionAdminParams.WITH_COLLECTION);
Review comment:
`withCollection` variable here will be the property configured on the collection, it will not resolve the alias if there's one. I believe we need to use params from the placement request to decide if we try to resolve?
----------------------------------------------------------------
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] madrob commented on a change in pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552160275
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
##########
@@ -59,13 +59,8 @@
import static org.apache.solr.common.cloud.ZkStateReader.READ_ONLY;
import static org.apache.solr.common.cloud.ZkStateReader.REPLICATION_FACTOR;
import static org.apache.solr.common.cloud.ZkStateReader.TLOG_REPLICAS;
-import static org.apache.solr.common.params.CollectionAdminParams.ALIAS;
-import static org.apache.solr.common.params.CollectionAdminParams.COLL_CONF;
-import static org.apache.solr.common.params.CollectionAdminParams.COUNT_PROP;
-import static org.apache.solr.common.params.CollectionAdminParams.CREATE_NODE_SET_PARAM;
-import static org.apache.solr.common.params.CollectionAdminParams.CREATE_NODE_SET_SHUFFLE_PARAM;
-import static org.apache.solr.common.params.CollectionAdminParams.ROUTER_PREFIX;
-import static org.apache.solr.common.params.CollectionAdminParams.SKIP_NODE_ASSIGNMENT;
+import static org.apache.solr.common.params.CollectionAdminParams.*;
Review comment:
please don't use wildcard imports
----------------------------------------------------------------
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 pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#issuecomment-754989462
**Blocking issue**: `CreateCollectionCmd` is not the only place where `withCollection` logic is going to get applied. All calls that end up calling the placement plugins now need to deal with returned placement for multiple collections: `SplitShardCmd`, `AddReplicaCmd`, `ReplaceNodeCmd`, `RestoreCmd`. Or at least ignore the additional replica assignments being returned.
I also see `CollectionsRepairEventListener`. We will also eventually have cluster rebalancers, will we expect them to support `withCollection`?
Dealing with replica placement for multiple collections returned from the placement plugins seems to deserve to be made a "first class" abstraction if we want to implement this (unreasonable?) feature in a reasonable way.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r553633390
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
Review comment:
We should not follow aliases at all and specify that the `withCollection` must be an existing real collection 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] sigram commented on a change in pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552527986
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -193,6 +195,22 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
Set<Node> nodes = request.getTargetNodes();
SolrCollection solrCollection = request.getCollection();
+ final SolrCollection secondaryCollection;
+ String withCollection = solrCollection.getCustomProperty(CollectionAdminParams.WITH_COLLECTION);
Review comment:
I think that we should completely disallow using aliases in this context... how would that work in practice? if you flip the alias that used to point to the secondary collection to some other collection then suddenly the co-location may be completely broken.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552216897
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
##########
@@ -80,6 +80,17 @@
*/
String COLL_CONF = "collection.configName";
+ /**
+ * The name of the collection with which a collection is to be co-located
Review comment:
Something along "Collection A with the attribute withCollection collection B implies two things: Collection B has only one shard, and on every node where collection A has a replica, there will also be a replica of collection B's shard".
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552198751
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
+ if (!clusterState.hasCollection(realWithCollection)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "The 'withCollection' does not exist: " + realWithCollection);
+ } else {
+ DocCollection collection = clusterState.getCollection(realWithCollection);
+ if (collection.getActiveSlices().size() > 1) {
Review comment:
Maybe test `!=1` to make sure it really does have one shard? Friendlier to fail with the `SolrException` here than below with `next()` throwing `NoSuchElementException`.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552215235
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -242,6 +260,37 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
}
}
+ if (secondaryCollection != null) {
Review comment:
Also, comment on that method that the approach is a rather simplistic one, since the set of nodes chosen for the placement request does not take into account the existing `secondaryCollection` distribution, possibly leading to the creation of replicas for that collection that could have been avoided.
I assume clusters where such `withCollection` tricks are done can restrict placement of both collections to a set of nodes to better control what's going on...
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552220473
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -254,8 +280,27 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
}
shardRequestTracker.processResponses(results, shardHandler, false, null, Collections.emptySet());
+
+ if (withCollection != null) {
+ // process replica placements for the secondary collection, if any are needed
+ for (ReplicaPosition replicaPosition : replicaPositions) {
+ if (!replicaPosition.collection.equals(withCollection)) {
+ continue;
+ }
+ ZkNodeProps props = new ZkNodeProps(
+ Overseer.QUEUE_OPERATION, ADDREPLICA.toString(),
+ ZkStateReader.COLLECTION_PROP, replicaPosition.collection,
+ ZkStateReader.SHARD_ID_PROP, replicaPosition.shard,
+ "node", replicaPosition.node,
+ CommonAdminParams.WAIT_FOR_FINAL_STATE, Boolean.TRUE.toString()); // set to true because we want `withCollection` to be ready after this collection is created
+ new AddReplicaCmd(ocmh).call(clusterState, props, results);
+ clusterState = zkStateReader.getClusterState(); // refresh
Review comment:
You know this call does absolutely nothing, right?
Good news is there's not need to refresh, not here and not in the other places where it's done.
When Collection API calls wait for something, they repeatedly read the in memory cluster state (maintained by ZkStateReader) until they observe the desired change. When they do it means that 1. the Overseer applied the change in ZK (directly as a cluster state change or as a side effect of a Collection API call itself doing a cluster state change as is the case here), and 2. the zookeeper watches that this node has on its cluster state did fire and the update from Zookeeper was applied to the cluster state.
It does happen that currently Collection API commands run on the Overseer node where the cluster state updater runs, but really the two are independent and could well be running on different nodes.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552214070
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -242,6 +260,37 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
}
}
+ if (secondaryCollection != null) {
+ // 2nd phase to allocate required secondary collection replicas
+ Set<Node> secondaryNodes = new HashSet<>();
+ Shard shard1 = secondaryCollection.iterator().next();
+ shard1.replicas().forEach(r -> {
+ secondaryNodes.add(r.getNode());
+ });
+ Set<ReplicaPlacement> secondaryPlacements = new HashSet<>();
+ Set<Node> alreadyAdded = new HashSet<>();
Review comment:
Can add the nodes directly into `secondaryNodes` and save introducing `alreadyAdded`, given the actual placement decisions are captures elsewhere in `secondaryPlacements`.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552193376
##########
File path: solr/solrj/src/java/org/apache/solr/common/cloud/ReplicaPosition.java
##########
@@ -19,17 +19,20 @@
public class ReplicaPosition implements Comparable<ReplicaPosition> {
Review comment:
It's not from this PR, but the text `Note: this class has a natural ordering that is inconsistent with equals.` should have been added to the class Javadoc that should have been here.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552224065
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
##########
@@ -80,6 +80,17 @@
*/
String COLL_CONF = "collection.configName";
+ /**
+ * The name of the collection with which a collection is to be co-located
Review comment:
And likely need to specify (here or elsewhere) that this is done for collection creation and adding replicas for Collection A, but not moving them...
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552526286
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
Review comment:
Frankly no idea :) that's the way it was done in 8x, there are probably edge cases either way. I'll add a note for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552210621
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -193,6 +195,22 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
Set<Node> nodes = request.getTargetNodes();
SolrCollection solrCollection = request.getCollection();
+ final SolrCollection secondaryCollection;
+ String withCollection = solrCollection.getCustomProperty(CollectionAdminParams.WITH_COLLECTION);
Review comment:
Taking this back, ignore previous comments. We resolved alias at collection creation time so expecting real collection name here.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -193,6 +195,22 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
Set<Node> nodes = request.getTargetNodes();
SolrCollection solrCollection = request.getCollection();
+ final SolrCollection secondaryCollection;
+ String withCollection = solrCollection.getCustomProperty(CollectionAdminParams.WITH_COLLECTION);
Review comment:
Taking this back, ignore previous comment.
We resolved alias at collection creation time so expecting real collection name here.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552196761
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
+ if (!clusterState.hasCollection(realWithCollection)) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "The 'withCollection' does not exist: " + realWithCollection);
+ } else {
+ DocCollection collection = clusterState.getCollection(realWithCollection);
+ if (collection.getActiveSlices().size() > 1) {
+ throw new SolrException(ErrorCode.BAD_REQUEST, "The `withCollection` must have only one shard, found: " + collection.getActiveSlices().size());
+ }
+ withCollectionShard = collection.getActiveSlices().iterator().next().getName();
Review comment:
Assigned but never used?
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552524187
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
##########
@@ -80,6 +80,17 @@
*/
String COLL_CONF = "collection.configName";
+ /**
+ * The name of the collection with which a collection is to be co-located
Review comment:
At this stage we're not fully sure yet how far this feature will go in ensuring this constraint, and where it will be implemented... This flag here just for indicating the constraint, implementation notes should go to the plugins that support this flag.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552212826
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -242,6 +260,37 @@ public PlacementPlan computePlacement(Cluster cluster, PlacementRequest request,
}
}
+ if (secondaryCollection != null) {
Review comment:
Can you extract the if bloc to a separate method?
----------------------------------------------------------------
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] HoustonPutman commented on a change in pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r553621627
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
##########
@@ -80,6 +80,17 @@
*/
String COLL_CONF = "collection.configName";
+ /**
+ * The name of the collection with which a collection is to be co-located
Review comment:
I think it's valid to specify and document what this flag does by default. Obviously plugins should add documentation on additional logic that they use for these flags as 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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552195844
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/CollectionAdminParams.java
##########
@@ -80,6 +80,17 @@
*/
String COLL_CONF = "collection.configName";
+ /**
+ * The name of the collection with which a collection is to be co-located
Review comment:
It would be helpful to explicit here what this really means and assumes.
----------------------------------------------------------------
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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r553633390
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
Review comment:
We should not follow aliases at all and specify that the `withCollection` must be an existing real collection name.
You said that below so deleting this 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 commented on pull request #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#issuecomment-785770902
This approach has been abandoned in favor of PR 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 #2179: SOLR-15055 Re-implement 'withCollection' and 'maxShardsPerNode'
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2179:
URL: https://github.com/apache/lucene-solr/pull/2179#discussion_r552201141
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java
##########
@@ -116,6 +121,25 @@ public void call(ClusterState clusterState, ZkNodeProps message, @SuppressWarnin
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "collection alias already exists: " + collectionName);
}
+ String withCollection = message.getStr(WITH_COLLECTION);
+ String withCollectionShard = null;
+ if (withCollection != null) {
+ String realWithCollection = aliases.resolveSimpleAlias(withCollection);
Review comment:
Shouldn't this only be done when message says `followAliases` is `true`?
----------------------------------------------------------------
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