Solr
  1. Solr
  2. SOLR-6214

Snapshots numberToKeep param only keeps n-1 backups

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.9
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      The numberToKeep param for snapshots doesn't work anymore. If you set the param to '2', only '1' backup is kept.

      In the ReplicationHandler in line 377 snapShooter.validateCreateSnapshot(); creates an empty directory for the new snapshot. The deleteOldBackups() method in Snapshooter which will be executed before the backup is created, now sees the two directories an deletes the old one. But this is wrong because the empty directory for the new backup should not be considered.

      1. SOLR-6214.patch
        6 kB
        Varun Thacker
      2. SOLR-6214.patch
        6 kB
        Varun Thacker
      3. SOLR-6214.patch
        9 kB
        Ramana

        Activity

        Hide
        Ramana added a comment - - edited

        Attached is the patch. Basically, "numberToKeep" is to indicate how many backups to retain (including this one). Reference http://wiki.apache.org/solr/SolrReplication

        Now, we are not considering the new snapshot directory while comparing "numberToKeep" parameter with existing directories size.

        if(numberToKeep > dirs.size()-1)

        { return; }

        with the changes in the patch, always "numbertokeep" parameter will be satisfied.

        Please verify.

        Show
        Ramana added a comment - - edited Attached is the patch. Basically, "numberToKeep" is to indicate how many backups to retain (including this one). Reference http://wiki.apache.org/solr/SolrReplication Now, we are not considering the new snapshot directory while comparing "numberToKeep" parameter with existing directories size. if(numberToKeep > dirs.size()-1) { return; } with the changes in the patch, always "numbertokeep" parameter will be satisfied. Please verify.
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Mathias and Ramana. We need a test as well before it can be committed.

        Show
        Shalin Shekhar Mangar added a comment - Thanks Mathias and Ramana. We need a test as well before it can be committed.
        Hide
        Ramana added a comment -

        Shalin, Updated the patch with test case. Please verify.

        Show
        Ramana added a comment - Shalin, Updated the patch with test case. Please verify.
        Hide
        Varun Thacker added a comment -
        File dir = new File(snapDir);
        if (!dir.exists())  dir.mkdirs();
        

        This is not needed because we call SimpleFSDirectory(destDir.toPath(), NoLockFactory.INSTANCE) which creates the snapDir if not present.

        Secondly I moved deleteOldBackups after the snapshot gets created. That makes sense right?

        Fixed the test to catch this regression. If you just run the test without making the fix to SnapShooter the test will fail.

        Show
        Varun Thacker added a comment - File dir = new File(snapDir); if (!dir.exists()) dir.mkdirs(); This is not needed because we call SimpleFSDirectory(destDir.toPath(), NoLockFactory.INSTANCE) which creates the snapDir if not present. Secondly I moved deleteOldBackups after the snapshot gets created. That makes sense right? Fixed the test to catch this regression. If you just run the test without making the fix to SnapShooter the test will fail.
        Hide
        Varun Thacker added a comment -

        Correct patch.

        Show
        Varun Thacker added a comment - Correct patch.
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks Mathias, Ramana and Varun!

        Show
        Shalin Shekhar Mangar added a comment - Thanks Mathias, Ramana and Varun!
        Hide
        ASF subversion and git services added a comment -

        Commit 1659180 from shalin@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1659180 ]

        SOLR-6214: Snapshots numberToKeep param only keeps n-1 backups

        Show
        ASF subversion and git services added a comment - Commit 1659180 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1659180 ] SOLR-6214 : Snapshots numberToKeep param only keeps n-1 backups
        Hide
        ASF subversion and git services added a comment -

        Commit 1659181 from shalin@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1659181 ]

        SOLR-6214: Snapshots numberToKeep param only keeps n-1 backups

        Show
        ASF subversion and git services added a comment - Commit 1659181 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1659181 ] SOLR-6214 : Snapshots numberToKeep param only keeps n-1 backups
        Hide
        Varun Thacker added a comment -

        Small typo in line 235 of TestReplicationHandlerBackup -

        fail("Backup should have been cleaned up because " + backupKeepParamName + " was set to 2."); should be
        fail("Backup should have been cleaned up because " + backupKeepParamName + " was set to 1.");

        Show
        Varun Thacker added a comment - Small typo in line 235 of TestReplicationHandlerBackup - fail("Backup should have been cleaned up because " + backupKeepParamName + " was set to 2."); should be fail("Backup should have been cleaned up because " + backupKeepParamName + " was set to 1.");
        Hide
        Shalin Shekhar Mangar added a comment -

        Fixed, thanks!

        Show
        Shalin Shekhar Mangar added a comment - Fixed, thanks!
        Hide
        ASF subversion and git services added a comment -

        Commit 1659203 from shalin@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1659203 ]

        SOLR-6214: Fix mistake in failure message

        Show
        ASF subversion and git services added a comment - Commit 1659203 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1659203 ] SOLR-6214 : Fix mistake in failure message
        Hide
        ASF subversion and git services added a comment -

        Commit 1659204 from shalin@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1659204 ]

        SOLR-6214: Fix mistake in failure message

        Show
        ASF subversion and git services added a comment - Commit 1659204 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1659204 ] SOLR-6214 : Fix mistake in failure message
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

          • Assignee:
            Shalin Shekhar Mangar
            Reporter:
            Mathias H.
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development