Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8423

DELETESHARD should cleanup the instance and data directory, like DELETEREPLICA

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0, 6.1, master (7.0)
    • Component/s: None
    • Labels:
      None

      Description

      DELETESHARD only cleans up the index directory and not the instance/data directory. DELETEREPLICA on the other hand cleans up the data and instance directory.

      DELETESHARD should clean up the instance and data directory, so that we don't leak disk space on executing the command.

      If we think this would break back-compat, though I don't see why this should not clean up the instance dir, we should at least provide an option to clean up everything and make it default in 6.0.

      1. SOLR-8423.patch
        17 kB
        Anshum Gupta
      2. SOLR-8423.patch
        3 kB
        Anshum Gupta
      3. SOLR-8423.patch
        2 kB
        Anshum Gupta

        Activity

        Hide
        erickerickson Erick Erickson added a comment -

        I don't think this breaks back-compat 'cause I don't see any promises in the docs But I vaguely recall that this was to clean up stale cluster states without having to directly edit the ZK node, so maybe nobody thought about it?

        I might advocate an option to keep at least the data directory just on general principles here though. I'm thinking of the case where the user is explicitly controlling routing, say time-series data. Does it make sense to delete/create shards to hide/show some time-interval? I have to admit this is a theoretical use-case, haven't seen anyone actually ask for it so feel free to ignore...

        Show
        erickerickson Erick Erickson added a comment - I don't think this breaks back-compat 'cause I don't see any promises in the docs But I vaguely recall that this was to clean up stale cluster states without having to directly edit the ZK node, so maybe nobody thought about it? I might advocate an option to keep at least the data directory just on general principles here though. I'm thinking of the case where the user is explicitly controlling routing, say time-series data. Does it make sense to delete/create shards to hide/show some time-interval? I have to admit this is a theoretical use-case, haven't seen anyone actually ask for it so feel free to ignore...
        Hide
        anshumg Anshum Gupta added a comment -

        Patch without a test. I'll add a test for this and also need to find a better param for supporting deletion without cleaning up of the instance directory (like right now). In this patch, it's called safedelete.

        Show
        anshumg Anshum Gupta added a comment - Patch without a test. I'll add a test for this and also need to find a better param for supporting deletion without cleaning up of the instance directory (like right now). In this patch, it's called safedelete .
        Hide
        shaie Shai Erera added a comment -

        So with this patch, the default is that we delete the index + instance + data directory, but if the user wants, he can request a safedelete and the behavior goes back to what it is today? I'm fine with that.

        Show
        shaie Shai Erera added a comment - So with this patch, the default is that we delete the index + instance + data directory, but if the user wants, he can request a safedelete and the behavior goes back to what it is today? I'm fine with that.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Why not just use the existing deleteInstanceDir and deleteDataDir core admin params (and default them to true)? It is not obvious to me what 'safedelete' means.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Why not just use the existing deleteInstanceDir and deleteDataDir core admin params (and default them to true)? It is not obvious to me what 'safedelete' means.
        Hide
        anshumg Anshum Gupta added a comment -

        I just wanted to keep it easy for users and I think adding a single flag is an easier option.
        Also, we want to set both those to true and I personally feel that deleteInstanceDir/deleteDataDir being set to true is a little less intuitive.

        Show
        anshumg Anshum Gupta added a comment - I just wanted to keep it easy for users and I think adding a single flag is an easier option. Also, we want to set both those to true and I personally feel that deleteInstanceDir/deleteDataDir being set to true is a little less intuitive.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        This is easy for the users. No need to remember what a new vague sounding parameter does. deleteInstanceDir/deleteDataDir is as explicit as it gets.

        What the hell is a safedelete anyway?

        1. We don't accidentally delete other's data?
        2. We don't delete our data?
        3. We ensure that data is actually deleted and cannot be recovered?

        If you really want to keep things simple for users, do what deletereplica already does i.e. deletes instance dir, data dir, index automatically as we did in SOLR-6072. No switch necessary to control that. If you want to implement a way to control what gets deleted, implement it for both deleteshard and deletereplica and even better, just have deleteshard internally call deletereplica and avoid the code duplication.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - This is easy for the users. No need to remember what a new vague sounding parameter does. deleteInstanceDir/deleteDataDir is as explicit as it gets. What the hell is a safedelete anyway? We don't accidentally delete other's data? We don't delete our data? We ensure that data is actually deleted and cannot be recovered? If you really want to keep things simple for users, do what deletereplica already does i.e. deletes instance dir, data dir, index automatically as we did in SOLR-6072 . No switch necessary to control that. If you want to implement a way to control what gets deleted, implement it for both deleteshard and deletereplica and even better, just have deleteshard internally call deletereplica and avoid the code duplication.
        Hide
        anshumg Anshum Gupta added a comment -

        As I said, the name of the param was just something I put in there.

        need to find a better param for supporting deletion without cleaning up of the instance directory (like right now). In this patch, it's called safedelete.

        As far as reusing the current params is concerned, I'll go with your suggestion and put up another patch. I used CoreAdmin calls directly because I didn't want to stack up more tasks for the Overseer. It however makes sense to just refactor the coreadmin call code and reuse that.

        P.S: The meaning of 'safedelete' would have been clear through the ref guide/documentation. The difference between instance/data/index directory can get a little complicated for users though, specially as we're trying to move away from Core admin calls being exposed to end users (via http api).

        Show
        anshumg Anshum Gupta added a comment - As I said, the name of the param was just something I put in there. need to find a better param for supporting deletion without cleaning up of the instance directory (like right now). In this patch, it's called safedelete. As far as reusing the current params is concerned, I'll go with your suggestion and put up another patch. I used CoreAdmin calls directly because I didn't want to stack up more tasks for the Overseer. It however makes sense to just refactor the coreadmin call code and reuse that. P.S: The meaning of 'safedelete' would have been clear through the ref guide/documentation. The difference between instance/data/index directory can get a little complicated for users though, specially as we're trying to move away from Core admin calls being exposed to end users (via http api).
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        deleteInstanceDir and deleteDataDir

        Probably best to be consistent in naming these params - especially given the behavior is the same. At least on 5x. On 6x, would still be nice to be consistent, but we could change how it works if we had ideas to improve it.

        Show
        markrmiller@gmail.com Mark Miller added a comment - deleteInstanceDir and deleteDataDir Probably best to be consistent in naming these params - especially given the behavior is the same. At least on 5x. On 6x, would still be nice to be consistent, but we could change how it works if we had ideas to improve it.
        Hide
        anshumg Anshum Gupta added a comment -

        Patch that uses deleteDataDir, deleteInstanceDir and defaults them to true.
        It now also supports setting those to false for DELETESHARD and DELETEREPLICA.

        Show
        anshumg Anshum Gupta added a comment - Patch that uses deleteDataDir, deleteInstanceDir and defaults them to true. It now also supports setting those to false for DELETESHARD and DELETEREPLICA.
        Hide
        shalinmangar Shalin Shekhar Mangar added a comment -

        Thanks Anshum. Looks good to me. We need a test though.

        Show
        shalinmangar Shalin Shekhar Mangar added a comment - Thanks Anshum. Looks good to me. We need a test though.
        Hide
        anshumg Anshum Gupta added a comment -

        Right, working on that. Got stuck up with something else.

        Show
        anshumg Anshum Gupta added a comment - Right, working on that. Got stuck up with something else.
        Hide
        anshumg Anshum Gupta added a comment -

        Patch with tests. I'll commit this once the 6.0 work is in place.

        Show
        anshumg Anshum Gupta added a comment - Patch with tests. I'll commit this once the 6.0 work is in place.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 9c777ab5adfd07e49310a5fb091d8bac611ef0ba in lucene-solr's branch refs/heads/master from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c777ab ]

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

        Show
        jira-bot ASF subversion and git services added a comment - Commit 9c777ab5adfd07e49310a5fb091d8bac611ef0ba in lucene-solr's branch refs/heads/master from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=9c777ab ] SOLR-8423 : DeleteShard and DeleteReplica should cleanup instance and data directory by default and add support for optionally retaining the directories
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 638b145376baea5281273bb90cedd8f69fecfa9f in lucene-solr's branch refs/heads/branch_6x from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=638b145 ]

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

        Show
        jira-bot ASF subversion and git services added a comment - Commit 638b145376baea5281273bb90cedd8f69fecfa9f in lucene-solr's branch refs/heads/branch_6x from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=638b145 ] SOLR-8423 : DeleteShard and DeleteReplica should cleanup instance and data directory by default and add support for optionally retaining the directories
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ca03639e6d5fbae924060fdb0b087189bb65a75d in lucene-solr's branch refs/heads/branch_6_0 from anshum
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ca03639 ]

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

        Show
        jira-bot ASF subversion and git services added a comment - Commit ca03639e6d5fbae924060fdb0b087189bb65a75d in lucene-solr's branch refs/heads/branch_6_0 from anshum [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ca03639 ] SOLR-8423 : DeleteShard and DeleteReplica should cleanup instance and data directory by default and add support for optionally retaining the directories
        Hide
        hossman Hoss Man added a comment -

        Anshum Gupta, Shalin Shekhar Mangar, Mark Miller - can one of you please review the ref guide edits i made for this issue...

        (It was a little confusing to me how to document this clearly since, AFAICT, all the params default to "true", regardless of what other params the user might set to "false", so setting deleteInstanceDir=false by itself doesn't seem to do anything very useful – the dataDir (and index) is still deleted because deleteDatadDir defaults to true.)

        Show
        hossman Hoss Man added a comment - Anshum Gupta , Shalin Shekhar Mangar , Mark Miller - can one of you please review the ref guide edits i made for this issue... https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604284&selectedPageVersions=156&selectedPageVersions=157 https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=61339928&selectedPageVersions=5&selectedPageVersions=6 (It was a little confusing to me how to document this clearly since, AFAICT, all the params default to "true", regardless of what other params the user might set to "false", so setting deleteInstanceDir=false by itself doesn't seem to do anything very useful – the dataDir (and index) is still deleted because deleteDatadDir defaults to true.)
        Hide
        anshumg Anshum Gupta added a comment -

        I'll take a look.

        Show
        anshumg Anshum Gupta added a comment - I'll take a look.
        Hide
        anshumg Anshum Gupta added a comment -

        You are right about deleteInstanceDir not doing anything by itself as the data directory would still be deleted by default and deleteDataDir has a similar issue. This calls for a JIRA but in terms of documenting this, we might have to ask users to set both of them to false for 6.0.

        Show
        anshumg Anshum Gupta added a comment - You are right about deleteInstanceDir not doing anything by itself as the data directory would still be deleted by default and deleteDataDir has a similar issue. This calls for a JIRA but in terms of documenting this, we might have to ask users to set both of them to false for 6.0.
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S6 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S6 of LUCENE-7271

          People

          • Assignee:
            anshumg Anshum Gupta
            Reporter:
            anshumg Anshum Gupta
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development