SOLR-8423: DeleteShard and DeleteReplica should cleanup instance and data directory by default and add support for optionally retaining the directories

solr/CHANGES.txt

@@ -386,6 +386,9 @@ Other Changes
 
 * SOLR-7010: Remove facet.date client functionality. (Steve Rowe)
 
+* SOLR-8423: DeleteShard and DeleteReplica should cleanup instance and data directory by default and add
+  support for optionally retaining the directories. (Anshum Gupta)
+
 ==================  5.5.1 ==================
 
 Bug Fixes

solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java

@@ -600,8 +600,10 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     ModifiableSolrParams params = new ModifiableSolrParams();
     params.add(CoreAdminParams.ACTION, CoreAdminAction.UNLOAD.toString());
     params.add(CoreAdminParams.CORE, core);
-    params.add(CoreAdminParams.DELETE_INSTANCE_DIR, "true");
+
-    params.add(CoreAdminParams.DELETE_DATA_DIR, "true");
+    params.set(CoreAdminParams.DELETE_INDEX, message.getBool(CoreAdminParams.DELETE_INDEX, true));
+    params.set(CoreAdminParams.DELETE_INSTANCE_DIR, message.getBool(CoreAdminParams.DELETE_INSTANCE_DIR, true));
+    params.set(CoreAdminParams.DELETE_DATA_DIR, message.getBool(CoreAdminParams.DELETE_DATA_DIR, true));
 
     sendShardRequest(replica.getNodeName(), params, shardHandler, asyncId, requestMap);
 
@@ -1407,7 +1409,10 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     try {
       ModifiableSolrParams params = new ModifiableSolrParams();
       params.set(CoreAdminParams.ACTION, CoreAdminAction.UNLOAD.toString());
-      params.set(CoreAdminParams.DELETE_INDEX, "true");
+      params.set(CoreAdminParams.DELETE_INDEX, message.getBool(CoreAdminParams.DELETE_INDEX, true));
+      params.set(CoreAdminParams.DELETE_INSTANCE_DIR, message.getBool(CoreAdminParams.DELETE_INSTANCE_DIR, true));
+      params.set(CoreAdminParams.DELETE_DATA_DIR, message.getBool(CoreAdminParams.DELETE_DATA_DIR, true));
+
       sliceCmd(clusterState, params, null, slice, shardHandler, asyncId, requestMap);
 
       processResponses(results, shardHandler, true, "Failed to delete shard", asyncId, requestMap, Collections.emptySet());

solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java

@@ -45,6 +45,9 @@ import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
 import static org.apache.solr.common.params.CommonParams.NAME;
 import static org.apache.solr.common.params.CommonParams.VALUE_LONG;
 import static org.apache.solr.common.params.CoreAdminParams.DATA_DIR;
+import static org.apache.solr.common.params.CoreAdminParams.DELETE_DATA_DIR;
+import static org.apache.solr.common.params.CoreAdminParams.DELETE_INDEX;
+import static org.apache.solr.common.params.CoreAdminParams.DELETE_INSTANCE_DIR;
 import static org.apache.solr.common.params.CoreAdminParams.INSTANCE_DIR;
 import static org.apache.solr.common.params.ShardParams._ROUTE_;
 import static org.apache.solr.common.util.StrUtils.formatString;
@@ -478,9 +481,14 @@ public class CollectionsHandler extends RequestHandlerBase {
     DELETESHARD_OP(DELETESHARD) {
       @Override
       Map<String, Object> call(SolrQueryRequest req, SolrQueryResponse rsp, CollectionsHandler handler) throws Exception {
-        return req.getParams().required().getAll(null,
+        Map<String, Object> map = req.getParams().required().getAll(null,
             COLLECTION_PROP,
             SHARD_ID_PROP);
+        req.getParams().getAll(map,
+            DELETE_INDEX,
+            DELETE_DATA_DIR,
+            DELETE_INSTANCE_DIR);
+        return map;
       }
     },
     FORCELEADER_OP(FORCELEADER) {
@@ -517,6 +525,12 @@ public class CollectionsHandler extends RequestHandlerBase {
             COLLECTION_PROP,
             SHARD_ID_PROP,
             REPLICA_PROP);
+
+        req.getParams().getAll(map,
+            DELETE_INDEX,
+            DELETE_DATA_DIR,
+            DELETE_INSTANCE_DIR);
+
         return req.getParams().getAll(map, ONLY_IF_DOWN);
       }
     },

solr/core/src/java/org/apache/solr/util/FileUtils.java

@@ -93,4 +93,7 @@ public class FileUtils {
       throw exc;
   }
 
+  public static boolean fileExists(String filePathString) {
+    return new File(filePathString).exists();
+  }
 }

solr/core/src/test/org/apache/solr/cloud/DeleteReplicaTest.java

@@ -16,10 +16,19 @@
  */
 package org.apache.solr.cloud;
 
+import java.io.File;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.CloudSolrClient;
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.CoreAdminRequest;
 import org.apache.solr.client.solrj.request.QueryRequest;
 import org.apache.solr.client.solrj.response.CoreAdminResponse;
@@ -31,24 +40,17 @@ import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.params.MapSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
+import org.apache.solr.util.FileUtils;
 import org.apache.solr.util.TimeOut;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.File;
-import java.io.IOException;
-import java.lang.invoke.MethodHandles;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.TimeUnit;
-
 import static org.apache.solr.cloud.OverseerCollectionMessageHandler.NUM_SLICES;
 import static org.apache.solr.cloud.OverseerCollectionMessageHandler.ONLY_IF_DOWN;
-import static org.apache.solr.common.util.Utils.makeMap;
 import static org.apache.solr.common.cloud.ZkStateReader.MAX_SHARDS_PER_NODE;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.DELETEREPLICA;
+import static org.apache.solr.common.util.Utils.makeMap;
 
 public class DeleteReplicaTest extends AbstractFullDistribZkTestBase {
 
@@ -170,4 +172,44 @@ public class DeleteReplicaTest extends AbstractFullDistribZkTestBase {
     Map<String,List<Integer>> collectionInfos = new HashMap<>();
     createCollection(collectionInfos, COLL_NAME, props, client);
   }
+
+  @Test
+  @ShardsFixed(num = 2)
+  public void deleteReplicaAndVerifyDirectoryCleanup() throws IOException, SolrServerException, InterruptedException {
+    createCollection("deletereplica_test", 1, 2, 4);
+
+    Replica leader = cloudClient.getZkStateReader().getLeaderRetry("deletereplica_test", "shard1");
+    String baseUrl = (String) leader.get("base_url");
+    String core = (String) leader.get("core");
+    String leaderCoreName = leader.getName();
+
+    String instanceDir;
+    String dataDir;
+
+    try (HttpSolrClient client = new HttpSolrClient(baseUrl)) {
+      CoreAdminResponse statusResp = CoreAdminRequest.getStatus(core, client);
+      NamedList r = statusResp.getCoreStatus().get(core);
+      instanceDir = (String) r.findRecursive("instanceDir");
+      dataDir = (String) r.get("dataDir");
+    }
+
+    //Confirm that the instance and data directory exist
+    assertTrue("Instance directory doesn't exist", FileUtils.fileExists(instanceDir));
+    assertTrue("DataDirectory doesn't exist", FileUtils.fileExists(dataDir));
+
+    new CollectionAdminRequest.DeleteReplica()
+        .setCollectionName("deletereplica_test")
+        .setShardName("shard1")
+        .setReplica(leaderCoreName)
+        .process(cloudClient);
+
+    Replica newLeader = cloudClient.getZkStateReader().getLeaderRetry("deletereplica_test", "shard1");
+
+    assertFalse(leader.equals(newLeader));
+
+    //Confirm that the instance and data directory were deleted by default
+
+    assertFalse("Instance directory still exists", FileUtils.fileExists(instanceDir));
+    assertFalse("DataDirectory still exists", FileUtils.fileExists(dataDir));
+  }
 }

solr/core/src/test/org/apache/solr/cloud/DeleteShardTest.java

@@ -16,27 +16,34 @@
  */
 package org.apache.solr.cloud;
 
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.CoreAdminRequest;
 import org.apache.solr.client.solrj.request.QueryRequest;
+import org.apache.solr.client.solrj.response.CollectionAdminResponse;
+import org.apache.solr.client.solrj.response.CoreAdminResponse;
 import org.apache.solr.cloud.overseer.OverseerAction;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica;
 import org.apache.solr.common.cloud.Slice;
 import org.apache.solr.common.cloud.Slice.State;
 import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.params.CollectionParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.Utils;
+import org.apache.solr.util.FileUtils;
 import org.apache.zookeeper.KeeperException;
 import org.junit.Test;
 
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
-
 public class DeleteShardTest extends AbstractFullDistribZkTestBase {
 
   public DeleteShardTest() {
@@ -150,4 +157,67 @@ public class DeleteShardTest extends AbstractFullDistribZkTestBase {
     }
   }
 
+  @Test
+  public void testDirectoryCleanupAfterDeleteShard() throws InterruptedException, IOException, SolrServerException {
+    CollectionAdminResponse rsp = new CollectionAdminRequest.Create()
+        .setCollectionName("deleteshard_test")
+        .setRouterName("implicit")
+        .setShards("a,b,c")
+        .setReplicationFactor(1)
+        .setConfigName("conf1")
+        .process(cloudClient);
+
+    // Get replica details
+    Replica leader = cloudClient.getZkStateReader().getLeaderRetry("deleteshard_test", "a");
+    String baseUrl = (String) leader.get("base_url");
+    String core = (String) leader.get("core");
+
+    String instanceDir;
+    String dataDir;
+
+    try (HttpSolrClient client = new HttpSolrClient(baseUrl)) {
+      CoreAdminResponse statusResp = CoreAdminRequest.getStatus(core, client);
+      NamedList r = statusResp.getCoreStatus().get(core);
+      instanceDir = (String) r.findRecursive("instanceDir");
+      dataDir = (String) r.get("dataDir");
+    }
+
+    assertTrue("Instance directory doesn't exist", FileUtils.fileExists(instanceDir));
+    assertTrue("Data directory doesn't exist", FileUtils.fileExists(dataDir));
+
+    assertEquals(3, cloudClient.getZkStateReader().getClusterState().getActiveSlices("deleteshard_test").size());
+
+    // Delete shard 'a'
+    new CollectionAdminRequest.DeleteShard()
+        .setCollectionName("deleteshard_test")
+        .setShardName("a")
+        .process(cloudClient);
+
+    assertEquals(2, cloudClient.getZkStateReader().getClusterState().getActiveSlices("deleteshard_test").size());
+    assertFalse("Instance directory still exists", FileUtils.fileExists(instanceDir));
+    assertFalse("Data directory still exists", FileUtils.fileExists(dataDir));
+
+    leader = cloudClient.getZkStateReader().getLeaderRetry("deleteshard_test", "b");
+    baseUrl = (String) leader.get("base_url");
+    core = (String) leader.get("core");
+
+    try (HttpSolrClient client = new HttpSolrClient(baseUrl)) {
+      CoreAdminResponse statusResp = CoreAdminRequest.getStatus(core, client);
+      NamedList r = statusResp.getCoreStatus().get(core);
+      instanceDir = (String) r.findRecursive("instanceDir");
+      dataDir = (String) r.get("dataDir");
+    }
+
+    // Delete shard 'b'
+    new CollectionAdminRequest.DeleteShard()
+        .setCollectionName("deleteshard_test")
+        .setShardName("b")
+        .setDeleteDataDir(false)
+        .setDeleteInstanceDir(false)
+        .process(cloudClient);
+
+    assertEquals(1, cloudClient.getZkStateReader().getClusterState().getActiveSlices("deleteshard_test").size());
+    assertTrue("Instance directory still exists", FileUtils.fileExists(instanceDir));
+    assertTrue("Data directory still exists", FileUtils.fileExists(dataDir));
+  }
 }

solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java

@@ -526,6 +526,10 @@ public abstract class CollectionAdminRequest <Q extends CollectionAdminRequest<Q
 
   // DELETESHARD request
   public static class DeleteShard extends CollectionShardAsyncAdminRequest<DeleteShard> {
+
+    private Boolean deleteInstanceDir;
+    private Boolean deleteDataDir;
+
     public DeleteShard() {
       action = CollectionAction.DELETESHARD;
     }
@@ -534,6 +538,36 @@ public abstract class CollectionAdminRequest <Q extends CollectionAdminRequest<Q
     protected DeleteShard getThis() {
       return this;
     }
+
+    public Boolean getDeleteInstanceDir() {
+      return deleteInstanceDir;
+    }
+
+    public DeleteShard setDeleteInstanceDir(Boolean deleteInstanceDir) {
+      this.deleteInstanceDir = deleteInstanceDir;
+      return this;
+    }
+
+    public Boolean getDeleteDataDir() {
+      return deleteDataDir;
+    }
+
+    public DeleteShard setDeleteDataDir(Boolean deleteDataDir) {
+      this.deleteDataDir = deleteDataDir;
+      return this;
+    }
+
+    @Override
+    public SolrParams getParams() {
+      ModifiableSolrParams params = new ModifiableSolrParams(super.getParams());
+      if (deleteInstanceDir != null) {
+        params.set(CoreAdminParams.DELETE_INSTANCE_DIR, deleteInstanceDir);
+      }
+      if (deleteDataDir != null) {
+        params.set(CoreAdminParams.DELETE_DATA_DIR, deleteDataDir);
+      }
+      return params;
+    }
   }
 
   // FORCELEADER request
@@ -793,6 +827,9 @@ public abstract class CollectionAdminRequest <Q extends CollectionAdminRequest<Q
   public static class DeleteReplica extends CollectionShardAsyncAdminRequest<DeleteReplica> {
     protected String replica;
     protected Boolean onlyIfDown;
+    private Boolean deleteDataDir;
+    private Boolean deleteInstanceDir;
+    private Boolean deleteIndexDir;
 
     public DeleteReplica() {
       action = CollectionAction.DELETEREPLICA;
@@ -824,6 +861,15 @@ public abstract class CollectionAdminRequest <Q extends CollectionAdminRequest<Q
       if (onlyIfDown != null) {
         params.set("onlyIfDown", onlyIfDown);
       }
+      if (deleteDataDir != null) {
+        params.set(CoreAdminParams.DELETE_DATA_DIR, deleteDataDir);
+      }
+      if (deleteInstanceDir != null) {
+        params.set(CoreAdminParams.DELETE_INSTANCE_DIR, deleteInstanceDir);
+      }
+      if (deleteIndexDir != null) {
+        params.set(CoreAdminParams.DELETE_INDEX, deleteIndexDir);
+      }
       return params;
     }
 
@@ -831,6 +877,24 @@ public abstract class CollectionAdminRequest <Q extends CollectionAdminRequest<Q
     protected DeleteReplica getThis() {
       return this;
     }
+
+    public Boolean getDeleteDataDir() {
+      return deleteDataDir;
+    }
+
+    public DeleteReplica setDeleteDataDir(Boolean deleteDataDir) {
+      this.deleteDataDir = deleteDataDir;
+      return this;
+    }
+
+    public Boolean getDeleteInstanceDir() {
+      return deleteInstanceDir;
+    }
+
+    public DeleteReplica setDeleteInstanceDir(Boolean deleteInstanceDir) {
+      this.deleteInstanceDir = deleteInstanceDir;
+      return this;
+    }
   }
 
   // CLUSTERPROP request