Solr
  1. Solr
  2. SOLR-4693

Create a collections API to delete/cleanup a shard

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      A collections API that unloads all replicas of a given shard and then removes it from the cluster state.

      It will remove only those shards which are INACTIVE or have no range (created for custom sharding)

      Among other places, this would be useful post the shard split call to manage the parent/original shard.

      1. SOLR-4693.patch
        25 kB
        Shalin Shekhar Mangar
      2. SOLR-4693.patch
        27 kB
        Shalin Shekhar Mangar
      3. SOLR-4693.patch
        22 kB
        Anshum Gupta
      4. SOLR-4693.patch
        13 kB
        Anshum Gupta
      5. SOLR-4693.patch
        7 kB
        Anshum Gupta

        Activity

        Hide
        Anshum Gupta added a comment -

        Working patch, minus the test.
        Will add the tests ASAP.

        Show
        Anshum Gupta added a comment - Working patch, minus the test. Will add the tests ASAP.
        Hide
        Anshum Gupta added a comment -

        Also, here's how the API call looks like:
        http://<host>/solr/admin/collections?action=DELETESHARD&shard=<shard_name>&name=<collection_name>

        This only allows for deletion of inactive slices to maintain integrity.
        Have added a todo to perhaps allow deletion of active slices as long as the HashRange for that slice is serviced by another active slice.

        Show
        Anshum Gupta added a comment - Also, here's how the API call looks like: http://<host>/solr/admin/collections?action=DELETESHARD&shard=<shard_name>&name=<collection_name> This only allows for deletion of inactive slices to maintain integrity. Have added a todo to perhaps allow deletion of active slices as long as the HashRange for that slice is serviced by another active slice.
        Hide
        Yonik Seeley added a comment -

        Have added a todo to perhaps allow deletion of active slices as long as the HashRange for that slice is serviced by another active slice.

        And we should allow deletion if the slice has no hash range (i.e. custom sharding)?

        Show
        Yonik Seeley added a comment - Have added a todo to perhaps allow deletion of active slices as long as the HashRange for that slice is serviced by another active slice. And we should allow deletion if the slice has no hash range (i.e. custom sharding)?
        Hide
        Anshum Gupta added a comment -

        And we should allow deletion if the slice has no hash range (i.e. custom sharding)?

        Makes sense, will add that as a todo. But for now, (primarily for shard split cleanup) we'll just let a user delete a Slice as long as the Slice is inactive.

        Show
        Anshum Gupta added a comment - And we should allow deletion if the slice has no hash range (i.e. custom sharding)? Makes sense, will add that as a todo. But for now, (primarily for shard split cleanup) we'll just let a user delete a Slice as long as the Slice is inactive.
        Hide
        Anshum Gupta added a comment -

        Patch with the test.
        As of now the API allows for the deletion of:
        1. INACTIVE Slices
        2. Slices which have no Range (Custom Hashing).

        We should cleanup the shard<->slice confusion and stick to something for all of SolrCloud.

        Show
        Anshum Gupta added a comment - Patch with the test. As of now the API allows for the deletion of: 1. INACTIVE Slices 2. Slices which have no Range (Custom Hashing). We should cleanup the shard<->slice confusion and stick to something for all of SolrCloud.
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Anshum.

        A few comments:

        1. Can we use "collection" instead of "name" just like we use in splitshard?
        2. The following code will throw an exception for a shard with no range (custom hashing use-case). Also it allows deletion of slices in construction state going against the error message.
              // For now, only allow for deletions of Inactive slices or custom hashes (range==null).
              // TODO: Add check for range gaps on Slice deletion
              if (!slice.getState().equals(Slice.INACTIVE) && slice.getRange() != null) {
                throw new SolrException(ErrorCode.BAD_REQUEST,
                    "The slice: " + slice.getName() + " is not currently "
                    + slice.getState() + ". Only inactive (or custom-hashed) slices can be deleted.");
              }
          
        3. The "deletecore" call to overseer is redundant because it is also made by the CoreAdmin UNLOAD action.
        4. Can we re-use the code between "deletecollection" and "deleteshard"? The collectionCmd code checks for "live" state as well.
        5. In DeleteSliceTest, after setSliceAsInactive(), we should poll the slice state until it becomes inactive or until a timeout value instead of just waiting for 5000ms
        6. DeleteSliceTest.waitAndConfirmSliceDeletion is wrong. It does not actually use the counter variable. Also, cloudClient.getZkStateReader().getClusterState() doesn't actually force refresh the cluster state
        7. We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here?
        8. Do we know what would happen if such a "zombie" node comes back up? We need to make sure it cleans up properly.
        Show
        Shalin Shekhar Mangar added a comment - Thanks Anshum. A few comments: Can we use "collection" instead of "name" just like we use in splitshard? The following code will throw an exception for a shard with no range (custom hashing use-case). Also it allows deletion of slices in construction state going against the error message. // For now, only allow for deletions of Inactive slices or custom hashes (range== null ). // TODO: Add check for range gaps on Slice deletion if (!slice.getState().equals(Slice.INACTIVE) && slice.getRange() != null ) { throw new SolrException(ErrorCode.BAD_REQUEST, "The slice: " + slice.getName() + " is not currently " + slice.getState() + ". Only inactive (or custom-hashed) slices can be deleted." ); } The "deletecore" call to overseer is redundant because it is also made by the CoreAdmin UNLOAD action. Can we re-use the code between "deletecollection" and "deleteshard"? The collectionCmd code checks for "live" state as well. In DeleteSliceTest, after setSliceAsInactive(), we should poll the slice state until it becomes inactive or until a timeout value instead of just waiting for 5000ms DeleteSliceTest.waitAndConfirmSliceDeletion is wrong. It does not actually use the counter variable. Also, cloudClient.getZkStateReader().getClusterState() doesn't actually force refresh the cluster state We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here? Do we know what would happen if such a "zombie" node comes back up? We need to make sure it cleans up properly.
        Hide
        Anshum Gupta added a comment -

        Thanks for the feedback Shalin Shekhar Mangar.
        The comments are w.r.t a patch that's almost complete, shall put it up in a few hours hopefully.

        Can we use "collection" instead of "name" just like we use in splitshard?

        Sure, changed. Actually, we could look at moving all APIs to use "collection" with 5.0 perhaps.

        Fixed the exception to have an OR (which short circuits and never ends up being an NPE)

        Can we re-use the code between "deletecollection" and "deleteshard"? The collectionCmd code checks for "live" state as well.

        We 'can' but I feel we shouldn't as shard deletion is specific to a Slice in a collection, the collection deletes all Slices. I think it'd be an overkill.

        DeleteSliceTest.waitAndConfirmSliceDeletion is wrong. It does not actually use the counter variable. Also, cloudClient.getZkStateReader().getClusterState() doesn't actually force refresh the cluster state

        The counter variable is incremented. The force refresh bit is fixed.

        In DeleteSliceTest, after setSliceAsInactive(), we should poll the slice state until it becomes inactive or until a timeout value instead of just waiting for 5000ms

        Done.

        We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here?

        Isn't this opposite of what you say in #3?

        Do we know what would happen if such a "zombie" node comes back up? We need to make sure it cleans up properly.

        Not sure of that behaviour. Wouldn't that be as good as a random shard coming up with an invalid/non-existent state? Do we currently know what would happen in that case, leaving the delete API aside? Will test it out anyways.

        Show
        Anshum Gupta added a comment - Thanks for the feedback Shalin Shekhar Mangar . The comments are w.r.t a patch that's almost complete, shall put it up in a few hours hopefully. Can we use "collection" instead of "name" just like we use in splitshard? Sure, changed. Actually, we could look at moving all APIs to use "collection" with 5.0 perhaps. Fixed the exception to have an OR (which short circuits and never ends up being an NPE) Can we re-use the code between "deletecollection" and "deleteshard"? The collectionCmd code checks for "live" state as well. We 'can' but I feel we shouldn't as shard deletion is specific to a Slice in a collection, the collection deletes all Slices. I think it'd be an overkill. DeleteSliceTest.waitAndConfirmSliceDeletion is wrong. It does not actually use the counter variable. Also, cloudClient.getZkStateReader().getClusterState() doesn't actually force refresh the cluster state The counter variable is incremented. The force refresh bit is fixed. In DeleteSliceTest, after setSliceAsInactive(), we should poll the slice state until it becomes inactive or until a timeout value instead of just waiting for 5000ms Done. We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here? Isn't this opposite of what you say in #3? Do we know what would happen if such a "zombie" node comes back up? We need to make sure it cleans up properly. Not sure of that behaviour. Wouldn't that be as good as a random shard coming up with an invalid/non-existent state? Do we currently know what would happen in that case, leaving the delete API aside? Will test it out anyways.
        Hide
        Shalin Shekhar Mangar added a comment -

        We 'can' but I feel we shouldn't as shard deletion is specific to a Slice in a collection, the collection deletes all Slices. I think it'd be an overkill.

        I was thinking of a method:

        private void sliceCmd(ClusterState clusterState, ModifiableSolrParams params, String stateMatcher, Slice slice)
        

        which can be refactored out of collectionCmd and used for delete shard as well.

        bq. We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here?

        Isn't this opposite of what you say in #3?

        Well, no. Look at what delete collection does in OverseerCollectionProcessor. It sends a collectionCmd to unload cores and then to make sure that a deleted collection does not ever come back, it makes a call to Overseer to remove the collection from ZK. We can do the same thing (and introduce a delete slice in overseer) or we could track the hosts on which the core unload failed and call Overseer's deletecore api directly.

        Show
        Shalin Shekhar Mangar added a comment - We 'can' but I feel we shouldn't as shard deletion is specific to a Slice in a collection, the collection deletes all Slices. I think it'd be an overkill. I was thinking of a method: private void sliceCmd(ClusterState clusterState, ModifiableSolrParams params, String stateMatcher, Slice slice) which can be refactored out of collectionCmd and used for delete shard as well. bq. We should fail with appropriate error message if there were nodes which could not be unloaded. Perhaps a separate "deletecore" call is appropriate here? Isn't this opposite of what you say in #3? Well, no. Look at what delete collection does in OverseerCollectionProcessor. It sends a collectionCmd to unload cores and then to make sure that a deleted collection does not ever come back, it makes a call to Overseer to remove the collection from ZK. We can do the same thing (and introduce a delete slice in overseer) or we could track the hosts on which the core unload failed and call Overseer's deletecore api directly.
        Hide
        Anshum Gupta added a comment -

        I guess I've integrated all changes that seemed fine to me.

        Show
        Anshum Gupta added a comment - I guess I've integrated all changes that seemed fine to me.
        Hide
        Shalin Shekhar Mangar added a comment -
        • I changed Overseer action and methods to deleteShard instead of deleteSlice, sliceCmd to shardCmd and DeleteSliceTest to DeleteShardTest (and related changes inside the test as well). I know it is confusing but we already have createshard and updateshardstate in Overseer and I didn't want to add more inconsistency.
        • In CollectionHandler.handleDeleteShardAction, I removed the name == null check because we used params.required().get which ensures that name can never be null. The createcollection api also makes the same mistake
        • The Overseer.deleteSlice/deleteShard was not atomic so I changed the following:
          Map<String, Slice> newSlices = coll.getSlicesMap();
          

          to

          Map<String, Slice> newSlices = new LinkedHashMap<String, Slice>(coll.getSlicesMap());
          
        • Added a wait loop to OCP.deleteShard like the one in delete collection
        • Fixed shardCount and sliceCount in DeleteShardTest constructor
        • Added a break to the wait loop inside DeleteShardTest.setSliceAsInactive and added a force update cluster state. Also an exception in if (!transition) is created but never thrown.
        • Removed redundant assert in DeleteShardTest.doTest because it can never be triggerred (because an exception is thrown from the wait loop in setSliceAsInactive)
          assertEquals("Shard1 is not inactive yet.", Slice.INACTIVE, slice1.getState());
          
        • Added a connection and read timeout to HttpSolrServer created in DeleteShardTest.deleteShard
        • I also took this opportunity to remove the timeout hack I had added to CollectionHandler for splitshard and had forgotten to remove it
        • Moved zkStateReader.updateClusterState(true) inside the wait loop of DeleteShardTest.confirmShardDeletion
        • Removed extra assert in DeleteShardTest.confirmShardDeletion for shard2 to be not null.

        I'll commit this tomorrow if there are no objections.

        Show
        Shalin Shekhar Mangar added a comment - I changed Overseer action and methods to deleteShard instead of deleteSlice, sliceCmd to shardCmd and DeleteSliceTest to DeleteShardTest (and related changes inside the test as well). I know it is confusing but we already have createshard and updateshardstate in Overseer and I didn't want to add more inconsistency. In CollectionHandler.handleDeleteShardAction, I removed the name == null check because we used params.required().get which ensures that name can never be null. The createcollection api also makes the same mistake The Overseer.deleteSlice/deleteShard was not atomic so I changed the following: Map< String , Slice> newSlices = coll.getSlicesMap(); to Map< String , Slice> newSlices = new LinkedHashMap< String , Slice>(coll.getSlicesMap()); Added a wait loop to OCP.deleteShard like the one in delete collection Fixed shardCount and sliceCount in DeleteShardTest constructor Added a break to the wait loop inside DeleteShardTest.setSliceAsInactive and added a force update cluster state. Also an exception in if (!transition) is created but never thrown. Removed redundant assert in DeleteShardTest.doTest because it can never be triggerred (because an exception is thrown from the wait loop in setSliceAsInactive) assertEquals( "Shard1 is not inactive yet." , Slice.INACTIVE, slice1.getState()); Added a connection and read timeout to HttpSolrServer created in DeleteShardTest.deleteShard I also took this opportunity to remove the timeout hack I had added to CollectionHandler for splitshard and had forgotten to remove it Moved zkStateReader.updateClusterState(true) inside the wait loop of DeleteShardTest.confirmShardDeletion Removed extra assert in DeleteShardTest.confirmShardDeletion for shard2 to be not null. I'll commit this tomorrow if there are no objections.
        Hide
        Anshum Gupta added a comment -

        I'll just go through the patch tonight. Also, here are some related JIRAs I created:
        Make the use of Slice and Shard consistent across the code and document base https://issues.apache.org/jira/browse/SOLR-4998

        Show
        Anshum Gupta added a comment - I'll just go through the patch tonight. Also, here are some related JIRAs I created: Make the use of Slice and Shard consistent across the code and document base https://issues.apache.org/jira/browse/SOLR-4998
        Hide
        Anshum Gupta added a comment -

        Looks good to me other than a small suggestion.
        You may want to have an overloaded handleResponse function that also accepts TIMEOUT and let all the default not bother to pass it.

        Show
        Anshum Gupta added a comment - Looks good to me other than a small suggestion. You may want to have an overloaded handleResponse function that also accepts TIMEOUT and let all the default not bother to pass it.
        Hide
        Shalin Shekhar Mangar added a comment -

        Patch with an overloaded CollectionHandler.handleResponse method which accepts timeout.

        Thanks Anshum.

        Show
        Shalin Shekhar Mangar added a comment - Patch with an overloaded CollectionHandler.handleResponse method which accepts timeout. Thanks Anshum.
        Hide
        ASF subversion and git services added a comment -

        Commit 1499655 from shalin@apache.org
        [ https://svn.apache.org/r1499655 ]

        SOLR-4693: A "deleteshard" collections API that unloads all replicas of a given shard and then removes it from the cluster state. It will remove only those shards which are INACTIVE or have no range (created for custom sharding).

        Show
        ASF subversion and git services added a comment - Commit 1499655 from shalin@apache.org [ https://svn.apache.org/r1499655 ] SOLR-4693 : A "deleteshard" collections API that unloads all replicas of a given shard and then removes it from the cluster state. It will remove only those shards which are INACTIVE or have no range (created for custom sharding).
        Hide
        ASF subversion and git services added a comment -

        Commit 1499656 from shalin@apache.org
        [ https://svn.apache.org/r1499656 ]

        SOLR-4693: A 'deleteshard' collections API that unloads all replicas of a given shard and then removes it from the cluster state. It will remove only those shards which are INACTIVE or have no range (created for custom sharding).

        Show
        ASF subversion and git services added a comment - Commit 1499656 from shalin@apache.org [ https://svn.apache.org/r1499656 ] SOLR-4693 : A 'deleteshard' collections API that unloads all replicas of a given shard and then removes it from the cluster state. It will remove only those shards which are INACTIVE or have no range (created for custom sharding).
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Anshum!

        Show
        Shalin Shekhar Mangar added a comment - Thanks Anshum!
        Hide
        Anshum Gupta added a comment - - edited

        Shalin Shekhar Mangar looks like you missed a few changes that you put up in your list above.
        Renaming of Slice->Shard stuff primarily.

        I'll fix those as a part of SOLR-4998.

        Show
        Anshum Gupta added a comment - - edited Shalin Shekhar Mangar looks like you missed a few changes that you put up in your list above. Renaming of Slice->Shard stuff primarily. I'll fix those as a part of SOLR-4998 .
        Hide
        Shalin Shekhar Mangar added a comment -

        Yes, there is sliceCmd in Overseer and some local variables but those were there in the patch that I had posted.

        Thanks for opening SOLR-4998

        Show
        Shalin Shekhar Mangar added a comment - Yes, there is sliceCmd in Overseer and some local variables but those were there in the patch that I had posted. Thanks for opening SOLR-4998
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Anshum Gupta
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development