This is an automated email from the ASF dual-hosted git repository.
epugh pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/solr.git
The following commit(s) were added to refs/heads/main by this push:
new 8c440065c87 S3 backup repository: stop writing and depending on
directory marker objects (#5128)
8c440065c87 is described below
commit 8c440065c87d12d72a413b16c67481e5aab31dfc
Author: Samuel Verstraete <[email protected]>
AuthorDate: Mon Oct 26 12:06:46 2026 +0100
S3 backup repository: stop writing and depending on directory marker
objects (#5128)
Co-authored-by: Samuel Verstraete <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>
Co-authored-by: Eric Pugh <[email protected]>
---
.../fix-s3-restore-missing-directory-marker.yml | 9 +
.../core/backup/repository/BackupRepository.java | 14 ++
.../repository/DelegatingBackupRepository.java | 5 +
.../java/org/apache/solr/handler/SnapShooter.java | 3 +-
.../solr/handler/admin/api/BackupAPIBase.java | 22 ++-
.../handler/admin/api/CreateCollectionBackup.java | 2 +-
.../solr/handler/admin/api/CreateCoreBackup.java | 5 +-
.../org/apache/solr/s3/S3BackupRepository.java | 6 +-
.../java/org/apache/solr/s3/S3StorageClient.java | 117 ++++---------
.../org/apache/solr/s3/S3BackupRepositoryTest.java | 69 +++++++-
.../apache/solr/s3/S3IncrementalBackupTest.java | 193 +++++++++++++++++++++
.../src/test/org/apache/solr/s3/S3PathsTest.java | 22 +++
.../test/org/apache/solr/s3/S3ReadWriteTest.java | 1 +
.../deployment-guide/pages/backup-restore.adoc | 12 +-
.../collections/AbstractBackupRepositoryTest.java | 26 ++-
15 files changed, 401 insertions(+), 105 deletions(-)
diff --git a/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml
b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml
new file mode 100644
index 00000000000..3c1274dfc20
--- /dev/null
+++ b/changelog/unreleased/fix-s3-restore-missing-directory-marker.yml
@@ -0,0 +1,9 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: The S3 backup repository no longer writes or depends on zero-byte
directory marker objects, so S3 backups that were copied with tools that drop
those markers (such as `aws s3 sync`) can be restored again. A backup can also
be written to an S3 location that does not exist yet.
+type: fixed
+authors:
+ - name: Samuel Verstraete
+ nick: elangelo
+links:
+ - name: PR#5128
+ url: https://github.com/apache/solr/pull/5128
diff --git
a/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java
b/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java
index b8f86af5e3e..3d13ea6ac53 100644
---
a/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java
+++
b/solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java
@@ -166,6 +166,20 @@ public interface BackupRepository extends
NamedListInitializedPlugin, Closeable
*/
void deleteDirectory(URI path) throws IOException;
+ /**
+ * Whether this repository can hold an empty directory.
+ *
+ * <p>Object stores such as S3 have no real directories: a directory only
exists while at least
+ * one file is stored under its path, and {@link #createDirectory(URI)}
persists nothing. Such
+ * repositories return false, which lets a backup be written to a location
that does not exist
+ * yet.
+ *
+ * @return true if a directory created by {@link #createDirectory(URI)}
exists while it is empty
+ */
+ default boolean supportsEmptyDirectories() {
+ return true;
+ }
+
/**
* Copy a file from specified <code>sourceDir</code> to the destination
repository (i.e. backup).
*
diff --git
a/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java
b/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java
index e3b27cb073c..54cd5845842 100644
---
a/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java
+++
b/solr/core/src/java/org/apache/solr/core/backup/repository/DelegatingBackupRepository.java
@@ -76,6 +76,11 @@ public class DelegatingBackupRepository extends
AbstractBackupRepository {
return delegate.exists(path);
}
+ @Override
+ public boolean supportsEmptyDirectories() {
+ return delegate.supportsEmptyDirectories();
+ }
+
@Override
public PathType getPathType(URI path) throws IOException {
return delegate.getPathType(path);
diff --git a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java
b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java
index 81b2e9bc397..9de40d225f1 100644
--- a/solr/core/src/java/org/apache/solr/handler/SnapShooter.java
+++ b/solr/core/src/java/org/apache/solr/handler/SnapShooter.java
@@ -151,7 +151,8 @@ public class SnapShooter {
public void validateCreateSnapshot() throws IOException {
// Note - Removed the current behavior of creating the directory hierarchy.
// Do we really need to provide this support?
- if (!backupRepo.exists(baseSnapDirPath)) {
+ // Checked even when a missing location is allowed, to fail early on an
unreachable repository
+ if (!backupRepo.exists(baseSnapDirPath) &&
backupRepo.supportsEmptyDirectories()) {
throw new SolrException(
SolrException.ErrorCode.BAD_REQUEST, " Directory does not exist: " +
snapshotDirPath);
}
diff --git
a/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java
b/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java
index b7028ff0405..14656547a5f 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/api/BackupAPIBase.java
@@ -46,6 +46,20 @@ public abstract class BackupAPIBase extends AdminAPIBase {
}
}
+ /**
+ * Like {@link #getAndValidateBackupLocation(String, String)}, for an API
that writes a new backup
+ * to the location. On a repository that can't hold empty directories, a
location that does not
+ * exist yet is accepted: it comes into existence with the first file of the
backup.
+ */
+ protected String getAndValidateNewBackupLocation(String repositoryName,
String location)
+ throws IOException {
+ try (final var repository = createBackupRepository(repositoryName)) {
+ location = getLocation(repository, location);
+ ensureBackupLocationExists(repository, location,
!repository.supportsEmptyDirectories());
+ return location;
+ }
+ }
+
protected String getAndValidateIncrementalBackupLocation(
String repositoryName, String location, String backupName) throws
IOException {
try (final var repository = createBackupRepository(repositoryName)) {
@@ -80,10 +94,12 @@ public abstract class BackupAPIBase extends AdminAPIBase {
+ " parameter or as a default repository property or as a cluster
property.");
}
- private void ensureBackupLocationExists(BackupRepository repository, String
location) {
+ private void ensureBackupLocationExists(
+ BackupRepository repository, String location, boolean allowMissing) {
final URI uri = repository.createDirectoryURI(location);
try {
- if (!repository.exists(uri)) {
+ // Checked even when a missing location is allowed, to fail early on an
unreachable repository
+ if (!repository.exists(uri) && !allowMissing) {
throw new SolrException(
SolrException.ErrorCode.SERVER_ERROR, "specified location " + uri
+ " does not exist.");
}
@@ -98,7 +114,7 @@ public abstract class BackupAPIBase extends AdminAPIBase {
private String getAndValidateBackupLocation(BackupRepository
backupRepository, String location)
throws IOException {
location = getLocation(backupRepository, location);
- ensureBackupLocationExists(backupRepository, location);
+ ensureBackupLocationExists(backupRepository, location, false);
return location;
}
diff --git
a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java
b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java
index 5e5661378b3..0b8d0a67078 100644
---
a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java
+++
b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollectionBackup.java
@@ -90,7 +90,7 @@ public class CreateCollectionBackup extends BackupAPIBase
implements CollectionB
collectionName, Boolean.TRUE.equals(requestBody.followAliases));
requestBody.location =
- getAndValidateBackupLocation(requestBody.repository,
requestBody.location);
+ getAndValidateNewBackupLocation(requestBody.repository,
requestBody.location);
if (requestBody.backupStrategy == null) {
requestBody.backupStrategy = CollectionAdminParams.COPY_FILES_STRATEGY;
diff --git
a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java
b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java
index 9299657daf8..d47260d9279 100644
--- a/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java
+++ b/solr/core/src/java/org/apache/solr/handler/admin/api/CreateCoreBackup.java
@@ -111,7 +111,10 @@ public class CreateCoreBackup extends CoreAdminAPIBase
implements CreateCoreBack
// already
// exist here if triggered via a collection backup on a shared
file system. Otherwise,
// perhaps the FS location isn't shared -- we want an error.
- if
(!snapShooter.getBackupRepository().exists(snapShooter.getLocation())) {
+ // A repository that can't hold empty directories, such as S3,
has no location until
+ // the first file is written to it, so a missing one is accepted
there.
+ if
(!snapShooter.getBackupRepository().exists(snapShooter.getLocation())
+ &&
snapShooter.getBackupRepository().supportsEmptyDirectories()) {
throw new SolrException(
SolrException.ErrorCode.BAD_REQUEST,
"Directory to contain snapshots doesn't exist: "
diff --git
a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java
b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java
index 23b1ed4c509..a4f8f14211e 100644
---
a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java
+++
b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3BackupRepository.java
@@ -146,6 +146,11 @@ public class S3BackupRepository extends
AbstractBackupRepository {
client.createDirectory(s3Path);
}
+ @Override
+ public boolean supportsEmptyDirectories() {
+ return false;
+ }
+
@Override
public void deleteDirectory(URI path) throws IOException {
Objects.requireNonNull(path, "cannot delete directory with a null URI");
@@ -285,7 +290,6 @@ public class S3BackupRepository extends
AbstractBackupRepository {
throw new CorruptIndexException("file is too small:" +
indexInput.length(), indexInput);
}
- client.createDirectory(getS3Path(dest));
try (OutputStream outputStream = client.pushStream(s3Path)) {
byte[] buffer = new byte[CHUNK_SIZE];
diff --git
a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
index bf076226685..527fb03b63b 100644
---
a/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
+++
b/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java
@@ -41,10 +41,8 @@ import
software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
import software.amazon.awssdk.awscore.exception.AwsServiceException;
import software.amazon.awssdk.awscore.retry.AwsRetryStrategy;
import software.amazon.awssdk.core.ResponseInputStream;
-import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.core.exception.SdkException;
import software.amazon.awssdk.core.retry.RetryMode;
-import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.http.apache.ProxyConfiguration;
@@ -64,7 +62,6 @@ import
software.amazon.awssdk.services.s3.model.HeadObjectResponse;
import software.amazon.awssdk.services.s3.model.NoSuchBucketException;
import software.amazon.awssdk.services.s3.model.NoSuchKeyException;
import software.amazon.awssdk.services.s3.model.ObjectIdentifier;
-import software.amazon.awssdk.services.s3.model.PutObjectRequest;
import software.amazon.awssdk.services.s3.model.S3Object;
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Iterable;
@@ -84,9 +81,6 @@ public class S3StorageClient {
// S3 has a hard limit of 1000 keys per batch delete request
private static final int MAX_KEYS_PER_BATCH_DELETE = 1000;
- // Metadata name used to identify flag directory entries in S3
- private static final String S3_DIR_CONTENT_TYPE = "application/x-directory";
-
// Error messages returned by S3 for a key not found.
private static final Set<String> NOT_FOUND_CODES = Set.of("NoSuchKey", "404
Not Found");
@@ -182,30 +176,13 @@ public class S3StorageClient {
return clientBuilder.build();
}
- /** Create a directory in S3, if it does not already exist. */
- void createDirectory(String path) throws S3Exception {
- String sanitizedDirPath = sanitizedDirPath(path);
-
- // Only create the directory if it does not already exist
- if (!pathExists(sanitizedDirPath)) {
- createDirectory(getParentDirectory(sanitizedDirPath));
- // TODO see https://issues.apache.org/jira/browse/SOLR-15359
- // throw new S3Exception("Parent directory doesn't exist,
path=" + path);
-
- try {
- // Create empty object with content type header
- PutObjectRequest putRequest =
- PutObjectRequest.builder()
- .bucket(bucketName)
- .contentType(S3_DIR_CONTENT_TYPE)
- .key(sanitizedDirPath)
- .build();
- s3Client.putObject(putRequest, RequestBody.empty());
- } catch (SdkClientException ase) {
- throw handleAmazonException(ase);
- }
- }
- }
+ /**
+ * No-op: S3 has no directories, a "directory" is just a key prefix that
exists as soon as an
+ * object is written under it. Older Solr versions wrote zero-byte marker
objects here, but those
+ * get lost when a bucket is copied with tools such as {@code aws s3 sync},
so nothing may depend
+ * on them.
+ */
+ void createDirectory(String path) {}
/**
* Delete files from S3. Deletion order is not guaranteed.
@@ -239,9 +216,6 @@ public class S3StorageClient {
// Get all the files and subdirectories
Set<String> entries = listAll(path);
- if (pathExists(path)) {
- entries.add(path);
- }
deleteObjects(entries);
}
@@ -292,10 +266,11 @@ public class S3StorageClient {
}
/**
- * Check if path exists.
+ * Check if path exists. A directory path (ending with {@code /}) exists if
any object is stored
+ * under that prefix, including a directory marker object written by older
Solr versions.
*
* @param path to File/Directory in S3.
- * @return true if path exists, otherwise false?
+ * @return true if path exists, otherwise false
*/
boolean pathExists(String path) throws S3Exception {
final String s3Path = sanitizedPath(path);
@@ -305,6 +280,10 @@ public class S3StorageClient {
return true;
}
+ if (s3Path.endsWith(S3_FILE_PATH_DELIMITER)) {
+ return hasObjectsUnderPrefix(s3Path);
+ }
+
try {
s3Client.headObject(builder -> builder.bucket(bucketName).key(s3Path));
return true;
@@ -316,23 +295,22 @@ public class S3StorageClient {
}
/**
- * Check if path is directory.
+ * Check if path is directory, i.e. if any object is stored under the {@code
path + "/"} prefix.
*
* @param path to File/Directory in S3.
* @return true if path is directory, otherwise false.
*/
boolean isDirectory(String path) throws S3Exception {
- final String s3Path = sanitizedDirPath(path);
+ return hasObjectsUnderPrefix(sanitizedDirPath(path));
+ }
+ private boolean hasObjectsUnderPrefix(String prefix) throws S3Exception {
try {
- HeadObjectResponse objectMetadata =
- s3Client.headObject(builder ->
builder.bucket(bucketName).key(s3Path));
- String contentType = objectMetadata.contentType();
-
- return StrUtils.isNotNullOrEmpty(contentType)
- && contentType.equalsIgnoreCase(S3_DIR_CONTENT_TYPE);
- } catch (NoSuchKeyException nske) {
- return false;
+ // listObjectsV2's prefix already guarantees every returned key starts
with it.
+ return !s3Client
+ .listObjectsV2(b -> b.bucket(bucketName).prefix(prefix).maxKeys(1))
+ .contents()
+ .isEmpty();
} catch (SdkException sdke) {
throw handleAmazonException(sdke);
}
@@ -346,19 +324,18 @@ public class S3StorageClient {
*/
long length(String path) throws S3Exception {
String s3Path = sanitizedFilePath(path);
- if (isDirectory(s3Path)) {
- throw new S3Exception("Path is Directory");
- }
try {
+ // sanitizedFilePath rejects a trailing '/', so a successful HEAD here
can never land on a
+ // directory marker object (whose key always ends in '/'); it's always a
real file.
HeadObjectResponse objectMetadata =
s3Client.headObject(b -> b.bucket(bucketName).key(s3Path));
- String contentType = objectMetadata.contentType();
-
- if (StrUtils.isNullOrEmpty(contentType)
- || !contentType.equalsIgnoreCase(S3_DIR_CONTENT_TYPE)) {
- return objectMetadata.contentLength();
+ return objectMetadata.contentLength();
+ } catch (NoSuchKeyException nske) {
+ // Only pay for the extra listing when the file is missing
+ if (isDirectory(s3Path)) {
+ throw new S3Exception("Path is Directory");
}
- throw new S3Exception("Path is Directory");
+ throw handleAmazonException(nske);
} catch (SdkException sdke) {
throw handleAmazonException(sdke);
}
@@ -409,10 +386,6 @@ public class S3StorageClient {
OutputStream pushStream(String path) throws S3Exception {
path = sanitizedFilePath(path);
- if (!parentDirectoryExist(path)) {
- throw new S3Exception("Parent directory doesn't exist of path: " + path);
- }
-
try {
return new S3OutputStream(s3Client, path, bucketName);
} catch (SdkException sdke) {
@@ -519,34 +492,6 @@ public class S3StorageClient {
}
}
- private boolean parentDirectoryExist(String path) throws S3Exception {
- // Get the last non-slash character of the string, to find the parent
directory
- String parentDirectory = getParentDirectory(path);
-
- // If we have no specific parent directory, we consider parent is root
(and always exists)
- if (parentDirectory.isEmpty() ||
parentDirectory.equals(S3_FILE_PATH_DELIMITER)) {
- return true;
- }
-
- // Check for existence twice, because s3Mock has issues in the tests
- return pathExists(parentDirectory);
- }
-
- private String getParentDirectory(String path) {
- if (!path.contains(S3_FILE_PATH_DELIMITER)) {
- return "";
- }
-
- // Get the last non-slash character of the string, to find the parent
directory
- int fromEnd = path.length() - 1;
- if (path.endsWith(S3_FILE_PATH_DELIMITER)) {
- fromEnd -= 1;
- }
- return fromEnd > 0
- ? path.substring(0, path.lastIndexOf(S3_FILE_PATH_DELIMITER, fromEnd)
+ 1)
- : "";
- }
-
/** Ensures path adheres to some rules: -Doesn't start with a leading slash
*/
String sanitizedPath(String path) {
// Trim space from start and end
diff --git
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
index bd24ac0a7cc..ae09e1ae8fd 100644
---
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
+++
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3BackupRepositoryTest.java
@@ -29,6 +29,8 @@ import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.time.Duration;
+import java.util.List;
+import java.util.Set;
import org.apache.commons.io.file.PathUtils;
import org.apache.lucene.codecs.CodecUtil;
import org.apache.lucene.store.BufferedIndexInput;
@@ -49,6 +51,7 @@ import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.S3Object;
@ThreadLeakFilters(
filters = {
@@ -97,8 +100,6 @@ public class S3BackupRepositoryTest extends
AbstractBackupRepositoryTest {
"s3:///d/",
directoryUri.toString());
- repo.createDirectory(directoryUri);
- assertTrue(repo.exists(directoryUri));
directoryUri = repo.createDirectoryURI("d/");
assertEquals(
"createDirectoryURI should have a single trailing slash, even if one
is provided",
@@ -118,15 +119,24 @@ public class S3BackupRepositoryTest extends
AbstractBackupRepositoryTest {
URI path = new URI("/test/");
repo.createDirectory(path);
- assertTrue(repo.exists(path));
- assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(path));
+ assertFalse("An empty directory doesn't exist on S3", repo.exists(path));
assertEquals("No files should exist in dir yet", 0,
repo.listAll(path).length);
URI subDir = new URI("/test/dir/");
repo.createDirectory(subDir);
+ assertEquals("No files should exist in subdir yet", 0,
repo.listAll(subDir).length);
+
+ try (OutputStream os = repo.createOutput(new URI("/test/dir/file"))) {
+ os.write(1);
+ }
+ assertTrue(repo.exists(path));
assertTrue(repo.exists(subDir));
+ assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(path));
assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(subDir));
- assertEquals("No files should exist in subdir yet", 0,
repo.listAll(subDir).length);
+ assertEquals(
+ "No directory marker objects should be written",
+ List.of("test/dir/file"),
+ listKeys("test/"));
assertEquals(
"subDir should now be returned when listing all in parent dir",
@@ -139,6 +149,36 @@ public class S3BackupRepositoryTest extends
AbstractBackupRepositoryTest {
}
}
+ /**
+ * Older Solr versions wrote a zero-byte marker object (content type {@code
+ * application/x-directory}) for every directory. Backups containing them
must keep working.
+ */
+ @Test
+ public void testLegacyDirectoryMarkers() throws Exception {
+ try (S3BackupRepository repo = getRepository()) {
+ pushDirectoryMarker("legacy/");
+ pushDirectoryMarker("legacy/empty/");
+ pushDirectoryMarker("legacy/dir/");
+ pushObject("legacy/dir/file", "content");
+
+ URI legacy = new URI("s3:/legacy/");
+ assertTrue(repo.exists(legacy));
+ assertTrue(repo.exists(repo.resolveDirectory(legacy, "empty")));
+ assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(legacy));
+ assertEquals(
+ BackupRepository.PathType.DIRECTORY,
+ repo.getPathType(repo.resolveDirectory(legacy, "dir")));
+ assertEquals(
+ BackupRepository.PathType.FILE,
repo.getPathType(repo.resolve(legacy, "dir", "file")));
+ assertEquals(Set.of("empty", "dir"), Set.of(repo.listAll(legacy)));
+ assertArrayEquals(new String[] {"file"},
repo.listAll(repo.resolveDirectory(legacy, "dir")));
+
+ repo.deleteDirectory(legacy);
+ assertFalse(repo.exists(legacy));
+ assertEquals("Directory markers should be deleted too", List.of(),
listKeys("legacy/"));
+ }
+ }
+
/** Check resolving paths. */
@Test
public void testResolve() throws Exception {
@@ -348,6 +388,25 @@ public class S3BackupRepositoryTest extends
AbstractBackupRepositoryTest {
}
}
+ private void pushDirectoryMarker(String path) {
+ try (S3Client s3 = s3MockContainer.createS3ClientV2()) {
+ s3.putObject(
+ b ->
b.bucket(BUCKET_NAME).key(path).contentType("application/x-directory"),
+ RequestBody.empty());
+ }
+ }
+
+ private List<String> listKeys(String prefix) {
+ try (S3Client s3 = s3MockContainer.createS3ClientV2()) {
+ return s3
+ .listObjectsV2Paginator(b -> b.bucket(BUCKET_NAME).prefix(prefix))
+ .contents()
+ .stream()
+ .map(S3Object::key)
+ .toList();
+ }
+ }
+
private Path pullObject(String path) throws IOException {
try (S3Client s3 = s3MockContainer.createS3ClientV2()) {
Path file = Files.createTempFile(temporaryFolder, "junit", null);
diff --git
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
index 9546012190b..cc3a4474d5c 100644
---
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
+++
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3IncrementalBackupTest.java
@@ -19,14 +19,33 @@ package org.apache.solr.s3;
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters;
import com.carrotsearch.randomizedtesting.annotations.ThreadLeakLingering;
+import java.util.List;
+import java.util.Map;
import org.apache.lucene.tests.util.LuceneTestCase;
import org.apache.lucene.tests.util.QuickPatchThreadsFilter;
import org.apache.solr.SolrIgnoredThreadsFilter;
+import org.apache.solr.client.solrj.SolrRequest.METHOD;
+import org.apache.solr.client.solrj.SolrRequest.SolrRequestType;
+import org.apache.solr.client.solrj.impl.CloudSolrClient;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.ConfigSetAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.client.solrj.request.SolrQuery;
+import org.apache.solr.client.solrj.response.RequestStatusState;
+import org.apache.solr.cloud.AbstractFullDistribZkTestBase;
import org.apache.solr.cloud.api.collections.AbstractIncrementalBackupTest;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.ZkStateReader;
+import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.handler.BackupRestoreUtils;
import org.apache.solr.util.LogLevel;
import org.junit.BeforeClass;
import org.junit.ClassRule;
+import org.junit.Test;
import software.amazon.awssdk.regions.Region;
+import software.amazon.awssdk.services.s3.S3Client;
+import software.amazon.awssdk.services.s3.model.S3Object;
// Backups do checksum validation against a footer value not present in
'SimpleText'
@LuceneTestCase.Nightly
@@ -141,4 +160,178 @@ public class S3IncrementalBackupTest extends
AbstractIncrementalBackupTest {
public String getBackupLocation() {
return "/";
}
+
+ /**
+ * Copying a backup to another bucket or prefix with tools such as {@code
aws s3 sync} only copies
+ * the real files, not zero-byte directory marker objects. Such a copy,
configset included, must
+ * still restore.
+ */
+ @Test
+ public void testRestoreBackupCopiedWithoutDirectoryMarkers() throws
Exception {
+ setTestSuffix("testcopiedwithoutmarkers");
+ final String backupCollectionName = getCollectionName();
+ final String restoreCollectionName = backupCollectionName + "_restore";
+ final String restoreConfigName = "conf1_restored";
+ final String backupName = BACKUPNAME_PREFIX + testSuffix;
+ final String copyLocation = "copied-backups";
+
+ CloudSolrClient solrClient = cluster.getSolrClient();
+ CollectionAdminRequest.createCollection(backupCollectionName, "conf1",
NUM_SHARDS, 1)
+ .process(solrClient);
+ int numDocs = indexDocs(backupCollectionName, true);
+ CollectionAdminRequest.backupCollection(backupCollectionName, backupName)
+ .setLocation(getBackupLocation())
+ .setRepositoryName(BACKUP_REPO_NAME)
+ .processAndWait(solrClient, 100);
+
+ try (S3Client s3 = s3MockContainer.createS3ClientV2()) {
+ List<String> keys =
+ s3
+ .listObjectsV2Paginator(b ->
b.bucket(BUCKET_NAME).prefix(backupName + "/"))
+ .contents()
+ .stream()
+ .map(S3Object::key)
+ .toList();
+ assertTrue(
+ "Backup should contain the configset",
+ keys.stream().anyMatch(k -> k.contains("/configs/conf1/")));
+ assertFalse(
+ "Backup should not contain directory marker objects: " + keys,
+ keys.stream().anyMatch(k -> k.endsWith("/")));
+
+ for (String key : keys) {
+ s3.copyObject(
+ b ->
+ b.sourceBucket(BUCKET_NAME)
+ .sourceKey(key)
+ .destinationBucket(BUCKET_NAME)
+ .destinationKey(copyLocation + "/" + key));
+ }
+ }
+
+ // Restoring under a new config name makes the restore upload the
configset from the backup
+ CollectionAdminRequest.restoreCollection(restoreCollectionName, backupName)
+ .setLocation(copyLocation)
+ .setConfigName(restoreConfigName)
+ .setRepositoryName(BACKUP_REPO_NAME)
+ .processAndWait(solrClient, 500);
+ AbstractFullDistribZkTestBase.waitForRecoveriesToFinish(
+ restoreCollectionName, ZkStateReader.from(solrClient), false, false,
3);
+
+ assertTrue(
+ new ConfigSetAdminRequest.List()
+ .process(solrClient)
+ .getConfigSets()
+ .contains(restoreConfigName));
+ assertEquals(
+ numDocs,
+ solrClient.query(restoreCollectionName, new
SolrQuery("*:*")).getResults().getNumFound());
+ }
+
+ /**
+ * S3 can't hold an empty directory, so a backup location doesn't have to be
created first: it
+ * comes into existence with the first file of the backup. Restoring from a
location that holds
+ * nothing still fails.
+ */
+ @Test
+ public void testBackupToLocationThatDoesNotExistYet() throws Exception {
+ setTestSuffix("testnewlocation");
+ final String backupCollectionName = getCollectionName();
+ final String restoreCollectionName = backupCollectionName + "_restore";
+ final String backupName = BACKUPNAME_PREFIX + testSuffix;
+ final String newLocation = "new-location/nested";
+
+ CloudSolrClient solrClient = cluster.getSolrClient();
+ CollectionAdminRequest.createCollection(backupCollectionName, "conf1",
NUM_SHARDS, 1)
+ .process(solrClient);
+ int numDocs = indexDocs(backupCollectionName, true);
+ assertEquals(
+ RequestStatusState.COMPLETED,
+ CollectionAdminRequest.backupCollection(backupCollectionName,
backupName)
+ .setLocation(newLocation)
+ .setRepositoryName(BACKUP_REPO_NAME)
+ .processAndWait(solrClient, 100));
+
+ assertEquals(
+ RequestStatusState.COMPLETED,
+ CollectionAdminRequest.restoreCollection(restoreCollectionName,
backupName)
+ .setLocation(newLocation)
+ .setRepositoryName(BACKUP_REPO_NAME)
+ .processAndWait(solrClient, 500));
+ AbstractFullDistribZkTestBase.waitForRecoveriesToFinish(
+ restoreCollectionName, ZkStateReader.from(solrClient), false, false,
3);
+ assertEquals(
+ numDocs,
+ solrClient.query(restoreCollectionName, new
SolrQuery("*:*")).getResults().getNumFound());
+
+ Exception e =
+ expectThrows(
+ Exception.class,
+ () ->
+ CollectionAdminRequest.restoreCollection(restoreCollectionName
+ "_2", backupName)
+ .setLocation("missing-location")
+ .setRepositoryName(BACKUP_REPO_NAME)
+ .processAndWait(solrClient, 100));
+ assertTrue(e.getMessage(), e.getMessage().contains("does not exist"));
+ }
+
+ /**
+ * A core backup in the non-incremental format to a location that does not
exist yet. On S3 the
+ * location only comes into existence with the first file of the backup.
+ */
+ @Test
+ public void testNonIncrementalCoreBackupToLocationThatDoesNotExistYet()
throws Exception {
+ setTestSuffix("testcorenewlocation");
+ final String collectionName = getCollectionName();
+ final String backupName = BACKUPNAME_PREFIX + testSuffix;
+ final String newLocation = "new-core-location/nested";
+
+ CloudSolrClient solrClient = cluster.getSolrClient();
+ CollectionAdminRequest.createCollection(collectionName, "conf1", 1,
1).process(solrClient);
+ int numDocs = indexDocs(collectionName, true);
+ Replica replica =
+
solrClient.getClusterState().getCollection(collectionName).getLeader("shard1");
+
+ runCoreAdminCommand(
+ replica,
+ CoreAdminAction.BACKUPCORE,
+ Map.of(
+ "name", backupName,
+ "repository", BACKUP_REPO_NAME,
+ "location", newLocation,
+ "incremental", "false"));
+
+ try (S3Client s3 = s3MockContainer.createS3ClientV2()) {
+ assertFalse(
+ "Backup should contain the snapshot's index files",
+ s3.listObjectsV2(
+ b -> b.bucket(BUCKET_NAME).prefix(newLocation + "/snapshot."
+ backupName + "/"))
+ .contents()
+ .isEmpty());
+ }
+
+ solrClient.deleteByQuery(collectionName, "*:*");
+ solrClient.commit(collectionName);
+ runCoreAdminCommand(
+ replica,
+ CoreAdminAction.RESTORECORE,
+ Map.of(
+ "name", "snapshot." + backupName,
+ "repository", BACKUP_REPO_NAME,
+ "location", newLocation));
+ BackupRestoreUtils.verifyDocs(numDocs, solrClient, collectionName);
+ }
+
+ private void runCoreAdminCommand(
+ Replica replica, CoreAdminAction action, Map<String, String> params)
throws Exception {
+ ModifiableSolrParams coreParams = new ModifiableSolrParams();
+ coreParams.set("action", action.toString());
+ coreParams.set("core", replica.getCoreName());
+ params.forEach(coreParams::set);
+ cluster
+ .getJetty(replica.getNodeName())
+ .getSolrClient()
+ .request(
+ new GenericSolrRequest(METHOD.GET, "/admin/cores",
SolrRequestType.ADMIN, coreParams));
+ }
}
diff --git
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java
index d314855c3b1..012b564b9e7 100644
--- a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java
+++ b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3PathsTest.java
@@ -48,6 +48,9 @@ public class S3PathsTest extends AbstractS3ClientTest {
public void testDirectory() throws S3Exception {
client.createDirectory("/simple-directory");
+ assertFalse("An empty dir does not exist on S3",
client.pathExists("/simple-directory/"));
+
+ pushContent("/simple-directory/a-file", "content");
assertFalse("Dir path needs a trailing slash",
client.pathExists("/simple-directory"));
assertTrue("Dir should exist without a leading slash",
client.pathExists("simple-directory/"));
assertTrue("Dir should exist with a leading slash",
client.pathExists("/simple-directory/"));
@@ -163,6 +166,25 @@ public class S3PathsTest extends AbstractS3ClientTest {
assertTrue(client.pathExists("/my-file2"));
}
+ /** A directory is any key prefix that has objects under it; no marker
object is needed. */
+ @Test
+ public void testVirtualDirectoryWithoutMarker() throws S3Exception {
+ pushContent("/virtual-dir-sibling", "not under the prefix");
+ pushContent("/virtual-dir/file1", "file1");
+ pushContent("/virtual-dir/sub-dir/file2", "file2");
+
+ assertTrue(
+ "A path with objects under it should be considered a virtual
directory",
+ client.isDirectory("/virtual-dir"));
+ assertTrue(
+ "A nested path with objects under it should be considered a virtual
directory",
+ client.isDirectory("/virtual-dir/sub-dir"));
+ assertFalse(
+ "A path with no objects and no marker should not be a directory",
+ client.isDirectory("/virtual-dir/empty-sub-dir"));
+ assertFalse("A file is not a directory",
client.isDirectory("/virtual-dir/file1"));
+ }
+
/** Check listing objects of a directory. */
@Test
public void testListDir() throws S3Exception {
diff --git
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
index 3574587b4fa..9ff6fa17858 100644
---
a/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
+++
b/solr/modules/s3-repository/src/test/org/apache/solr/s3/S3ReadWriteTest.java
@@ -90,6 +90,7 @@ public class S3ReadWriteTest extends AbstractS3ClientTest {
@Test
public void testDirectoryLength() throws Exception {
client.createDirectory("/directory");
+ pushContent("/directory/file", "child");
S3Exception exception =
assertThrows(
diff --git
a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc
b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc
index a0158af5a73..1a59d6d4877 100644
--- a/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc
+++ b/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc
@@ -772,14 +772,18 @@ When using the Backup & Restore Collections API Calls,
the **`location`** option
+
All the above options will resolve to the same directory in your S3 Bucket:
`dir/in/bucket`.
-Pre-Creation::
-The location must already exist within your S3 bucket before you can perform
any backup operations with that location.
-+
-**Please note that the directory you create in S3 cannot begin with a `/`, as
`locations` are stripped of any `/` prefix (shown in `Location Format`).**
+Location Creation::
+The location doesn't need to exist prior to backup; it is created
automatically when the first file is written.
+However, restoring, listing, or deleting backups requires an existing location
with at least one object.
Empty Location::
If you do not want to use a sub-directory within your bucket to store your
backup, you can use any of the following location options: `/`, `s3:/`, `s3://`.
However the location option is mandatory and you will receive an error when
trying to perform backup operations without it.
+
+Directory Markers::
+Backups can be copied between buckets or locations with standard S3 tools,
such as `aws s3 sync`, and still be restored.
+This works because Solr treats a directory as nothing more than the common key
prefix of the files stored under it, rather than writing a zero-byte "directory
marker" object for it.
+Backups made by older Solr versions, which do contain marker objects, remain
fully restorable and deletable.
====
An example configuration to enable S3 backups and restore can be seen below:
diff --git
a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java
b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java
index af8f5dbc3ce..eebdab230ef 100644
---
a/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java
+++
b/solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractBackupRepositoryTest.java
@@ -111,7 +111,7 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
repo.createDirectory(nonEmptyDirUri);
addFile(repo, nestedFileUri);
- assertTrue(repo.exists(emptyDirUri));
+ assertEquals(repo.supportsEmptyDirectories(), repo.exists(emptyDirUri));
assertTrue(repo.exists(nonEmptyDirUri));
assertTrue(repo.exists(nestedFileUri));
final URI nonexistedDirUri = repo.resolve(getBaseUri(),
"nonexistentDir/");
@@ -129,7 +129,9 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
repo.createDirectory(nonEmptyDirUri);
addFile(repo, nestedFileUri);
- assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(emptyDirUri));
+ if (repo.supportsEmptyDirectories()) {
+ assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(emptyDirUri));
+ }
assertEquals(BackupRepository.PathType.DIRECTORY,
repo.getPathType(nonEmptyDirUri));
assertEquals(BackupRepository.PathType.FILE,
repo.getPathType(nestedFileUri));
}
@@ -186,6 +188,8 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
repo.createDirectory(level2DeeplyNestedUri);
repo.createDirectory(level3DeeplyNestedUri);
repo.createDirectory(level4DeeplyNestedUri);
+ // Keep nest2 (and therefore nest1) non-empty once nest3 is deleted
+ keepNonEmptyIfNeeded(repo, level2DeeplyNestedUri, level4DeeplyNestedUri);
repo.deleteDirectory(level3DeeplyNestedUri);
assertTrue(repo.exists(level1DeeplyNestedUri));
assertTrue(repo.exists(level2DeeplyNestedUri));
@@ -241,6 +245,8 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
repo.createDirectory(otherDir1Uri);
repo.createDirectory(otherDir2Uri);
repo.createDirectory(otherDir3Uri);
+ // Without empty directories, otherDir1 and otherDir2 only show up once
they hold a file
+ keepNonEmptyIfNeeded(repo, otherDir1Uri, otherDir2Uri);
addFile(repo, file1Uri);
addFile(repo, file2Uri);
@@ -251,7 +257,7 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
assertTrue(rootChildren.contains("otherDir3"));
final String[] otherDir2Children = repo.listAll(otherDir2Uri);
- assertEquals(0, otherDir2Children.length);
+ assertEquals(repo.supportsEmptyDirectories() ? 0 : 1,
otherDir2Children.length);
final List<String> otherDir3Children =
List.of(repo.listAll(otherDir3Uri));
assertEquals(2, otherDir3Children.size());
@@ -391,6 +397,20 @@ public abstract class AbstractBackupRepositoryTest extends
SolrTestCaseJ4 {
}
}
+ /**
+ * On a repository that can't hold empty directories, seeds a file under
each given directory so
+ * it (and, transitively, its ancestors) stays non-empty for the rest of the
test. No-op when the
+ * repository supports real empty directories.
+ */
+ private void keepNonEmptyIfNeeded(BackupRepository repo, URI... dirs) throws
IOException {
+ if (repo.supportsEmptyDirectories()) {
+ return;
+ }
+ for (URI dir : dirs) {
+ addFile(repo, repo.resolve(dir, "file.txt"));
+ }
+ }
+
/**
* Test implementation of a {@link DelegatingBackupRepository} that disables
the checksum
* verification on its delegate {@link BackupRepository}.
