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/02/15 17:09:27 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #2373: SOLR-15130: Allow per-collection replica placement node sets
sigram opened a new pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373
Details in Jira.
This branch is based on `jira/solr-15131` so that is uses collection properties for node type settings.
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576364981
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
That's the most serious downside of keeping this config in collection properties - there's no easy way to keep the reverse mapping up to date. We could try updating the secondary collection... although this would require adding special case code to `OCMH.modifyCollection()`, which breaks the isolation of the plugin from the core.
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576340130
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
This loop is frightening... Looping over all collection will be expensive. Can't we instead update collection properties with the "reverse" of the `withCollection` relation?
Or that means that for high scale SolrCloud clusters (WIP), `withCollection` is strongly discouraged.
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java
##########
@@ -292,6 +303,17 @@ public boolean equals(Object obj) {
public int hashCode() {
return Objects.hash(shardName, collection, shardState);
}
+
+ @Override
+ public String toString() {
+ return "ShardImpl{" +
+ "shardName='" + shardName + '\'' +
Review comment:
Why quote the shard name and not the collection name?
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/impl/SimpleClusterAbstractionsImpl.java
##########
@@ -432,5 +454,17 @@ public boolean equals(Object obj) {
public int hashCode() {
return Objects.hash(replicaName, coreName, shard, replicaType, replicaState, node);
}
+
+ @Override
+ public String toString() {
+ return "ReplicaImpl{" +
+ "replicaName='" + replicaName + '\'' +
+ ", coreName='" + coreName + '\'' +
Review comment:
quoting strategy on returned string unclear
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#issuecomment-784547727
I'm not going to pursue this implementation - instead see PR 2421.
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576921425
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
I disagree, but your call.
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576921425
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
I disagree, but your call.
[Edit for clarification: either way you decide to eventually do it is ok for me because none is perfect and likely a matter of personal preference]
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576863376
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
Hmm, then at this point it makes more sense to keep this configuration in the plugin config :(
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373
----------------------------------------------------------------
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 #2373: SOLR-15130: Allow per-collection replica placement node sets
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #2373:
URL: https://github.com/apache/lucene-solr/pull/2373#discussion_r576411785
##########
File path: solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java
##########
@@ -280,25 +250,31 @@ public void verifyAllowedModification(ModificationRequest modificationRequest, P
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 +
- " of " + deleteCollectionRequest.getCollection().getName() + " still present");
- }
- } catch (IOException e) {
- throw new PlacementModificationException("failed to retrieve colocated collection information", e);
+ Set<String> colocatedWith = new HashSet<>();
+ for (SolrCollection collection : cluster.collections()) {
Review comment:
We could document to the user that in order to support safe deletes, the secondary collection must reference the primary one. Then up to the user to decide if the two collections are updated or not...
At least this leaves a scalable option available.
The other "cleaner" solution is to make collection references a first class citizen managed correctly (so we can maintain mapping and reverse mapping), but then the scope increases greatly and changes quite a few things in how collection are manages (and `withCollection` would then be just one specific usage of this collection reference mechanism, but maybe the only one...).
No perfect solution here, but once we get to very large numbers of collections (with collection lists not stored in node memory) the iteration will be prohibitively expensive (that's why I didn't want having such an iterator in the first place BTW).
----------------------------------------------------------------
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