diff --git a/changelog/unreleased/SOLR-18303-incremental-backup-cleanup-masks-error.yml b/changelog/unreleased/SOLR-18303-incremental-backup-cleanup-masks-error.yml new file mode 100644 index 00000000000..73132e57536 --- /dev/null +++ b/changelog/unreleased/SOLR-18303-incremental-backup-cleanup-masks-error.yml @@ -0,0 +1,8 @@ +# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc +title: Incremental backup failures now report the original error instead of an unrelated missing zk_backup directory exception +type: fixed +authors: + - name: Prithvi S +links: + - name: SOLR-18303 + url: https://issues.apache.org/jira/browse/SOLR-18303 diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java index f1ee97f58db..d7e8e21e309 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java @@ -133,8 +133,16 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList "Error happened during incremental backup for collection: {}", collectionName, e); - CollectionHandlingUtils.cleanBackup( - repository, backupUri, backupMgr.getBackupId(), ccc); + try { + CollectionHandlingUtils.cleanBackup( + repository, backupUri, backupMgr.getBackupId(), ccc); + } catch (Exception cleanupException) { + log.warn( + "Unable to clean up incomplete backup for collection: {}", + collectionName, + cleanupException); + e.addSuppressed(cleanupException); + } throw e; } } else { diff --git a/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteBackupCmd.java b/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteBackupCmd.java index 6b81d28cf0c..6c0ec870bf1 100644 --- a/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteBackupCmd.java +++ b/solr/core/src/java/org/apache/solr/cloud/api/collections/DeleteBackupCmd.java @@ -25,6 +25,7 @@ import java.io.IOException; import java.lang.invoke.MethodHandles; import java.net.URI; +import java.nio.file.NoSuchFileException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -206,13 +207,14 @@ void deleteBackupIds( .map(ShardBackupId::getBackupMetadataFilename) .collect(Collectors.toList())); repository.delete(incBackupFiles.getIndexDir(), unusedFiles); - try { - for (BackupId backupId : backupIdsDeletes) { - repository.deleteDirectory( - repository.resolveDirectory(backupUri, BackupFilePaths.getZkStateDir(backupId))); + for (BackupId backupId : backupIdsDeletes) { + URI zkStateDir = + repository.resolveDirectory(backupUri, BackupFilePaths.getZkStateDir(backupId)); + try { + repository.deleteDirectory(zkStateDir); + } catch (FileNotFoundException | NoSuchFileException e) { + // zk_backup_* is created after shard copy, so a failed incremental backup may not have it } - } catch (FileNotFoundException e) { - // ignore this } // add details to result before deleting backupPropFiles diff --git a/solr/core/src/test/org/apache/solr/cloud/api/collections/DeleteBackupCmdTest.java b/solr/core/src/test/org/apache/solr/cloud/api/collections/DeleteBackupCmdTest.java new file mode 100644 index 00000000000..5d01b5ce3af --- /dev/null +++ b/solr/core/src/test/org/apache/solr/cloud/api/collections/DeleteBackupCmdTest.java @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.cloud.api.collections; + +import java.io.IOException; +import java.net.URI; +import java.util.Set; +import java.util.UUID; +import org.apache.solr.SolrTestCase; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.core.backup.BackupFilePaths; +import org.apache.solr.core.backup.BackupId; +import org.apache.solr.core.backup.repository.BackupRepository; +import org.apache.solr.core.backup.repository.LocalFileSystemRepository; +import org.junit.Before; +import org.junit.Test; + +/** Unit tests for {@link DeleteBackupCmd}. */ +public class DeleteBackupCmdTest extends SolrTestCase { + + private BackupRepository repository; + private URI backupUri; + + @Before + public void setUpRepo() throws Exception { + repository = new LocalFileSystemRepository(); + backupUri = + repository.createDirectoryURI( + createTempDir("backup_" + UUID.randomUUID()).toAbsolutePath().toString()); + new BackupFilePaths(repository, backupUri).createIncrementalBackupFolders(); + } + + @Test + public void testDeleteBackupIdsIgnoresMissingZkStateDir() throws Exception { + NamedList results = new NamedList<>(); + new DeleteBackupCmd(null) + .deleteBackupIds(backupUri, repository, Set.of(BackupId.zero()), results); + + assertNotNull(results.get("deleted")); + assertFalse(repository.exists(zkStateDir(BackupId.zero()))); + } + + @Test + public void testDeleteBackupIdsRemovesExistingZkStateDir() throws Exception { + URI zkStateDir = zkStateDir(BackupId.zero()); + repository.createDirectory(zkStateDir); + assertTrue(repository.exists(zkStateDir)); + + new DeleteBackupCmd(null) + .deleteBackupIds(backupUri, repository, Set.of(BackupId.zero()), new NamedList<>()); + + assertFalse(repository.exists(zkStateDir)); + } + + @Test + public void testDeleteBackupIdsPropagatesUnexpectedDeleteErrors() { + BackupRepository failingRepository = + new LocalFileSystemRepository() { + @Override + public void deleteDirectory(URI path) throws IOException { + throw new IOException("simulated repository failure"); + } + }; + + IOException thrown = + expectThrows( + IOException.class, + () -> + new DeleteBackupCmd(null) + .deleteBackupIds( + backupUri, failingRepository, Set.of(BackupId.zero()), new NamedList<>())); + assertEquals("simulated repository failure", thrown.getMessage()); + } + + private URI zkStateDir(BackupId backupId) { + return repository.resolveDirectory(backupUri, BackupFilePaths.getZkStateDir(backupId)); + } +}