diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 14f80a3d290..4ba50b95256 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -95,7 +95,7 @@ Bug Fixes * SOLR-11821: ConcurrentModificationException in SimSolrCloudTestCase.tearDown (shalin) * SOLR-11631: The Schema API should return non-zero status when there are failures. - (Noble Paul, Steve Rowe) + (Noble Paul, Steve Rowe) Optimizations ---------------------- @@ -134,6 +134,8 @@ Other Changes * SOLR-11692: SolrDispatchFilter's use of a "close shield" in tests should not be applied to further servlet chain processing. (Jeff Miller, David Smiley) +* SOLR-11218: Fail and return an error when attempting to delete a collection that's part of an alias (Erick Erickson) + ================== 7.2.1 ================== Bug Fixes diff --git a/solr/core/src/java/org/apache/solr/cloud/DeleteCollectionCmd.java b/solr/core/src/java/org/apache/solr/cloud/DeleteCollectionCmd.java index dc91905ed37..8ee0168bd3e 100644 --- a/solr/core/src/java/org/apache/solr/cloud/DeleteCollectionCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/DeleteCollectionCmd.java @@ -21,14 +21,15 @@ package org.apache.solr.cloud; import java.lang.invoke.MethodHandles; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; import org.apache.solr.common.NonExistentCoreException; import org.apache.solr.common.SolrException; +import org.apache.solr.common.cloud.Aliases; import org.apache.solr.common.cloud.ClusterState; -import org.apache.solr.common.cloud.DocCollection; import org.apache.solr.common.cloud.SolrZkClient; import org.apache.solr.common.cloud.ZkNodeProps; import org.apache.solr.common.cloud.ZkStateReader; @@ -60,9 +61,15 @@ public class DeleteCollectionCmd implements OverseerCollectionMessageHandler.Cmd @Override public void call(ClusterState state, ZkNodeProps message, NamedList results) throws Exception { ZkStateReader zkStateReader = ocmh.zkStateReader; + Aliases aliases = zkStateReader.getAliases(); final String collection = message.getStr(NAME); - DocCollection coll = state.getCollectionOrNull(collection); - String policy = coll == null ? null : coll.getPolicyName(); + for (Map.Entry> ent : aliases.getCollectionAliasListMap().entrySet()) { + if (ent.getValue().contains(collection)) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, + "Collection : " + collection + " is part of alias " + ent.getKey() + " remove or modify the alias before removing this collection."); + } + } + try { // Remove the snapshots meta-data for this collection in ZK. Deleting actual index files // should be taken care of as part of collection delete operation. diff --git a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java index d0f0e80b196..1c20b30a5e4 100644 --- a/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java @@ -32,6 +32,7 @@ import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.UpdateRequest; import org.apache.solr.client.solrj.request.V2Request; import org.apache.solr.client.solrj.response.QueryResponse; +import org.apache.solr.client.solrj.response.RequestStatusState; import org.apache.solr.common.SolrException; import org.apache.solr.common.cloud.Aliases; import org.apache.solr.common.cloud.SolrZkClient; @@ -176,7 +177,163 @@ public class AliasIntegrationTest extends SolrCloudTestCase { } } } - + + // Rather a long title, but it's common to recommend when people need to re-index for any reason that they: + // 1> create a new collection + // 2> index the corpus to the new collection and verify it + // 3> create an alias pointing to the new collection WITH THE SAME NAME as their original collection + // 4> delete the old collection. + // + // They may or may not have an alias already pointing to the old collection that's being replaced. + // If they don't already have an alias, this leaves them with: + // + // > a collection named old_collection + // > a collection named new_collection + // > an alias old_collection->new_collection + // + // What happens when they delete old_collection now? + // + // Current behavior is that delete "does the right thing" and deletes old_collection rather than new_collection, + // but if this behavior changes it could be disastrous for users so this test insures that this behavior. + // + @Test + public void testDeleteAliasWithExistingCollectionName() throws Exception { + CollectionAdminRequest.createCollection("collection_old", "conf", 2, 1).process(cluster.getSolrClient()); + CollectionAdminRequest.createCollection("collection_new", "conf", 1, 1).process(cluster.getSolrClient()); + waitForState("Expected collection_old to be created with 2 shards and 1 replica", "collection_old", clusterShape(2, 1)); + waitForState("Expected collection_new to be created with 1 shard and 1 replica", "collection_new", clusterShape(1, 1)); + + new UpdateRequest() + .add("id", "6", "a_t", "humpty dumpy sat on a wall") + .add("id", "7", "a_t", "humpty dumpy3 sat on a walls") + .add("id", "8", "a_t", "humpty dumpy2 sat on a walled") + .commit(cluster.getSolrClient(), "collection_old"); + + new UpdateRequest() + .add("id", "1", "a_t", "humpty dumpy sat on an unfortunate wall") + .commit(cluster.getSolrClient(), "collection_new"); + + QueryResponse res = cluster.getSolrClient().query("collection_old", new SolrQuery("*:*")); + assertEquals(3, res.getResults().getNumFound()); + + // Let's insure we have a "handle" to the old collection + CollectionAdminRequest.createAlias("collection_old_reserve", "collection_old").process(cluster.getSolrClient()); + + // This is the critical bit. The alias uses the _old collection name. + CollectionAdminRequest.createAlias("collection_old", "collection_new").process(cluster.getSolrClient()); + + // aliases: collection_old->collection_new, collection_old_reserve -> collection_old -> collection_new + // collections: collection_new and collection_old + + // Now we should only see the doc in collection_new through the collection_old alias + res = cluster.getSolrClient().query("collection_old", new SolrQuery("*:*")); + assertEquals(1, res.getResults().getNumFound()); + + // Now we should still transitively see collection_new + res = cluster.getSolrClient().query("collection_old_reserve", new SolrQuery("*:*")); + assertEquals(1, res.getResults().getNumFound()); + + // Now delete the old collection. This should fail since the collection_old_reserve points to collection_old + RequestStatusState delResp = CollectionAdminRequest.deleteCollection("collection_old").processAndWait(cluster.getSolrClient(), 60); + assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); + + // assure ourselves that the old colletion is, indeed, still there. + assertNotNull("collection_old should exist!", cluster.getSolrClient().getZkStateReader().getClusterState().getCollectionOrNull("collection_old")); + + // Now we should still succeed using the alias collection_old which points to collection_new + // aliase: collection_old -> collection_new, collection_old_reserve -> collection_old -> collection_new + // collections: collection_old, collection_new + res = cluster.getSolrClient().query("collection_old", new SolrQuery("*:*")); + assertEquals(1, res.getResults().getNumFound()); + + Aliases aliases = cluster.getSolrClient().getZkStateReader().getAliases(); + assertTrue("collection_old should point to collection_new", aliases.resolveAliases("collection_old").contains("collection_new")); + assertTrue("collection_old_reserve should point to collection_new", aliases.resolveAliases("collection_old_reserve").contains("collection_new")); + + // Clean up + CollectionAdminRequest.deleteAlias("collection_old_reserve").processAndWait(cluster.getSolrClient(), 60); + CollectionAdminRequest.deleteAlias("collection_old").processAndWait(cluster.getSolrClient(), 60); + CollectionAdminRequest.deleteCollection("collection_new").processAndWait(cluster.getSolrClient(), 60); + CollectionAdminRequest.deleteCollection("collection_old").processAndWait(cluster.getSolrClient(), 60); + // collection_old already deleted as well as collection_old_reserve + + assertNull("collection_old_reserve should be gone", cluster.getSolrClient().getZkStateReader().getAliases().getCollectionAliasMap().get("collection_old_reserve")); + assertNull("collection_old should be gone", cluster.getSolrClient().getZkStateReader().getAliases().getCollectionAliasMap().get("collection_old")); + + assertFalse("collection_new should be gone", + cluster.getSolrClient().getZkStateReader().getClusterState().hasCollection("collection_new")); + + assertFalse("collection_old should be gone", + cluster.getSolrClient().getZkStateReader().getClusterState().hasCollection("collection_old")); + } + + // While writing the above test I wondered what happens when an alias points to two collections and one of them + // is deleted. + @Test + public void testDeleteOneOfTwoCollectionsAliased() throws Exception { + CollectionAdminRequest.createCollection("collection_one", "conf", 2, 1).process(cluster.getSolrClient()); + CollectionAdminRequest.createCollection("collection_two", "conf", 1, 1).process(cluster.getSolrClient()); + waitForState("Expected collection_one to be created with 2 shards and 1 replica", "collection_one", clusterShape(2, 1)); + waitForState("Expected collection_two to be created with 1 shard and 1 replica", "collection_two", clusterShape(1, 1)); + + new UpdateRequest() + .add("id", "1", "a_t", "humpty dumpy sat on a wall") + .commit(cluster.getSolrClient(), "collection_one"); + + + new UpdateRequest() + .add("id", "10", "a_t", "humpty dumpy sat on a high wall") + .add("id", "11", "a_t", "humpty dumpy sat on a low wall") + .commit(cluster.getSolrClient(), "collection_two"); + + // Create an alias pointing to both + CollectionAdminRequest.createAlias("collection_alias_pair", "collection_one,collection_two").process(cluster.getSolrClient()); + + QueryResponse res = cluster.getSolrClient().query("collection_alias_pair", new SolrQuery("*:*")); + assertEquals(3, res.getResults().getNumFound()); + + // Now delete one of the collections, should fail since an alias points to it. + RequestStatusState delResp = CollectionAdminRequest.deleteCollection("collection_one").processAndWait(cluster.getSolrClient(), 60); + + assertEquals("Should have failed to delete collection: ", delResp, RequestStatusState.FAILED); + + // Now redefine the alias to only point to colletion two + CollectionAdminRequest.createAlias("collection_alias_pair", "collection_two").process(cluster.getSolrClient()); + + //Delete collection_one. + delResp = CollectionAdminRequest.deleteCollection("collection_one").processAndWait(cluster.getSolrClient(), 60); + + assertEquals("Should not have failed to delete collection, it was removed from the alias: ", delResp, RequestStatusState.COMPLETED); + + // Should only see two docs now in second collection + res = cluster.getSolrClient().query("collection_alias_pair", new SolrQuery("*:*")); + assertEquals(2, res.getResults().getNumFound()); + + // We shouldn't be able to ping the deleted collection directly as + // was deleted (and, assuming that it only points to collection_old). + try { + cluster.getSolrClient().query("collection_one", new SolrQuery("*:*")); + } catch (SolrServerException se) { + assertTrue(se.getMessage().contains("No live SolrServers")); + } + + // Clean up + CollectionAdminRequest.deleteAlias("collection_alias_pair").processAndWait(cluster.getSolrClient(), 60); + CollectionAdminRequest.deleteCollection("collection_two").processAndWait(cluster.getSolrClient(), 60); + // collection_one already deleted + + assertNull("collection_alias_pair should be gone", + cluster.getSolrClient().getZkStateReader().getAliases().getCollectionAliasMap().get("collection_alias_pair")); + + assertFalse("collection_one should be gone", + cluster.getSolrClient().getZkStateReader().getClusterState().hasCollection("collection_one")); + + assertFalse("collection_two should be gone", + cluster.getSolrClient().getZkStateReader().getClusterState().hasCollection("collection_two")); + + } + + @Test public void test() throws Exception { CollectionAdminRequest.createCollection("collection1", "conf", 2, 1).process(cluster.getSolrClient()); @@ -332,6 +489,7 @@ public class AliasIntegrationTest extends SolrCloudTestCase { } } + @Test public void testErrorChecks() throws Exception { CollectionAdminRequest.createCollection("testErrorChecks-collection", "conf", 2, 1).process(cluster.getSolrClient()); waitForState("Expected testErrorChecks-collection to be created with 2 shards and 1 replica", "testErrorChecks-collection", clusterShape(2, 1)); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java index c0580ed674c..9539846f883 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java @@ -219,29 +219,6 @@ public class CloudSolrClientTest extends SolrCloudTestCase { assertEquals(2, client.query(null, paramsWithMixedCollectionAndAlias).getResults().getNumFound()); } - @Test - public void testHandlingOfStaleAlias() throws Exception { - CloudSolrClient client = getRandomClient(); - - CollectionAdminRequest.createCollection("nemesis", "conf", 2, 1).process(client); - CollectionAdminRequest.createAlias("misconfigured-alias", "nemesis").process(client); - CollectionAdminRequest.deleteCollection("nemesis").process(client); - - List docs = new ArrayList<>(); - - SolrInputDocument doc = new SolrInputDocument(); - doc.addField(id, Integer.toString(1)); - docs.add(doc); - - try { - client.add("misconfigured-alias", docs); - fail("Alias points to non-existing collection, add should fail"); - } catch (SolrException e) { - assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); - assertTrue("Unexpected exception", e.getMessage().contains("Collection not found")); - } - } - @Test public void testRouting() throws Exception {