fix(backup): handle S3-style object stores in config restore by elangelo · Pull Request #4182 · apache/solr

elangelo · 2026-03-03T16:03:06Z

Description

When restoring a backup, use both exists() and listAll() to check for the config directory. Object stores like S3 may not have an explicit directory marker object, but the config files are still present under that prefix. The previous check using only exists() would incorrectly throw an error in such cases.

Solution

See description

For transparency Github Copilot was used to identify and fix the issue we were encountering with this functionality.

Tests

Manually verified on our solrcloud clusters

Checklist

Please review the following and check all that apply:

[ x ] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
[ x ] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
[ x ] I have developed this patch against the main branch.
[ x ] I have run ./gradlew check.
[ x ] I have added tests for my changes.
[ x ] I have added documentation for the Reference Guide
[ x ] I have added a changelog entry for my change

When restoring a backup, use both `exists()` and `listAll()` to check for the config directory. Object stores like S3 may not have an explicit directory marker object, but the config files are still present under that prefix. The previous check using only `exists()` would incorrectly throw an error in such cases.

…configset When a backup is copied through a local filesystem and re-uploaded to S3 (e.g. via `aws s3 sync`), the zero-byte directory marker objects created by S3BackupRepository are typically lost. This caused restore to fail with an error even though all configset files were present under the expected key prefix. Change BackupManager#uploadConfigDir to fall back to listing objects by key prefix when the directory marker is absent, so restore succeeds as long as the configset files themselves are present. A clear IllegalArgumentException is still thrown when neither marker nor files can be found. - Add unit tests covering the no-marker, truly-absent, and normal cases - Add documentation in the S3 backup/restore reference guide explaining the directory-marker behaviour and the graceful fallback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

epugh · 2026-03-06T01:21:27Z

@@ -672,6 +672,14 @@ The location must already exist within your S3 bucket before you can perform any
 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::
+S3 has no native concept of directories.


Is there some better approach that we SHOULD be using instead of this zero-byte marker objects appraoch? If you were doing this today, is this how you would fix it? It feels like you are doing a bug fix because of a more fundamentally flawed approach? Or, is this the way we do these typs of things?

Well... I got sucked into this without indeed looking at the bigger picture. I'm not saying it's absolutely wrong to work with PREMARKER blobs, but it would certainly not how I would implement it.
The whole 'check if a file exists' is an anti-pattern anyway when working with 'cloud storage'. As even to check this you actually have to pay for that requests. It's way better to just try and deal with the error in case there is one.
I'm gonna look if it's possible to implement the way the backups themselves are taken and basically completely forget about the whole PREMARKER stuff. This should still be backwards compatibly with current backups that people rely on. The only restriction is that I will have to look into how the whole storage abstraction and see if I can make it work for both local and cloud storage without violating their way of working.

epugh · 2026-03-06T01:22:16Z

+    ZkStateReader mockZkStateReader = mock(ZkStateReader.class);
+    ConfigSetService mockConfigSetService = mock(ConfigSetService.class);
+
+    // Simulate S3: no marker object → exists() is false, but listAll() finds the files


maybe just me, but I find mocks hard to understand. We have S3Mock as a dependency, could we be using it for these tests?

I'll have a look into this. I missed that.

S3StorageClient.isDirectory: when no explicit directory-marker object exists (NoSuchKeyException), fall back to listing up to one object under the path prefix. This handles "virtual directories" that arise after an `aws s3 sync` round-trip, which silently drops zero-byte directory marker keys, causing restore to fail with NoSuchKey 404. CloudSolrClient: before computeIfAbsent for collectionRefreshes, evict any already-completed future via computeIfPresent. Without this, a stale done future is reused instead of submitting a fresh refresh, so callers (e.g. stale-state retry) never observe an updated ref. Also adds a unit test covering the virtual-directory fallback path and a changelog entry for the S3 fix.

Replace all uses of `repository.exists()` and explicit `createDirectory()` calls with virtual-directory convention throughout the backup/restore pipeline. S3 does not require zero-byte marker objects for directories. Writing such markers is an anti-pattern: tools like `aws s3 sync` silently drop them, causing subsequent `exists()` checks to return false and breaking restore. Changes: - `S3StorageClient.createDirectory()` is now a no-op (no marker written) - Remove `parentDirectoryExist()` guard from `pushStream()`; files can be written directly under any prefix - Replace `repository.exists()` checks with `listAllOrEmpty()` length checks in `BackupCmd`, `BackupManager`, and `SnapShooter` - `BackupFilePaths.createIncrementalBackupFolders()` and `BackupManager.createZkStateDir()` become no-ops - `BackupManager.downloadConfigDir()` no longer pre-creates directories - Consolidate two duplicate changelog entries into one Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…avior S3 does not use explicit directory markers, so `exists()` on a directory URI returns false even after `createDirectory()` — a directory is only implied by the objects it contains. - Update S3BackupRepositoryTest assertions to expect `false` from `exists()` on directories, and verify content is still listable - Override `testCanDetermineWhetherFilesAndDirectoriesExist`, `testCanDistinguishBetweenFilesAndDirectories`, `testCanDeleteEmptyOrFullDirectories`, and `testCanListFullOrEmptyDirectories` with S3-specific expectations (no directory marker objects) - Add `testSkipConfigset` override in S3IncrementalBackupTest to assert no explicit directory entries are created during S3 backup, while still verifying configset files are present/absent based on the backup flag - Fix S3ReadWriteTest#testDirectoryLength to push a child file into the directory before testing, satisfying S3's implicit directory semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-05-18T00:33:56Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

github-actions · 2026-07-18T00:25:13Z

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.

Address review on PR apache#4182: - Keep core backup/restore code unchanged: it still calls createDirectory() and exists(), so LocalFileSystemRepository (which needs real parent directories) keeps working. The S3 repository decides what a directory is. - S3StorageClient: createDirectory() is a no-op; a directory is a key prefix with objects under it. exists() on a directory path and isDirectory() use a single prefix listing, which also finds legacy marker objects, so old backups still restore and get fully deleted. length() is back to a single HEAD request for regular files. - Replace the Mockito-based BackupManagerUploadConfigTest with S3Mock tests: an end-to-end restore (configset included) of a backup copied without marker objects, and a test for backups containing legacy markers. - AbstractBackupRepositoryTest: add supportsEmptyDirectories() so S3 can run the shared contract tests instead of overriding them. - Drop the unrelated CloudSolrClient change; upstream rewrote that code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

elangelo · 2026-10-14T08:42:00Z

Continued in #5128. As discussed with @epugh above, it drops the directory marker objects altogether instead of working around missing ones, and the new tests use S3Mock instead of Mockito mocks. This PR couldn't be reopened after the branch was rebased onto main.

Address review on PR apache#4182: - Keep core backup/restore code unchanged: it still calls createDirectory() and exists(), so LocalFileSystemRepository (which needs real parent directories) keeps working. The S3 repository decides what a directory is. - S3StorageClient: createDirectory() is a no-op; a directory is a key prefix with objects under it. exists() on a directory path and isDirectory() use a single prefix listing, which also finds legacy marker objects, so old backups still restore and get fully deleted. length() is back to a single HEAD request for regular files. - Replace the Mockito-based BackupManagerUploadConfigTest with S3Mock tests: an end-to-end restore (configset included) of a backup copied without marker objects, and a test for backups containing legacy markers. - AbstractBackupRepositoryTest: add supportsEmptyDirectories() so S3 can run the shared contract tests instead of overriding them. - Drop the unrelated CloudSolrClient change; upstream rewrote that code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

samuelverstraete and others added 2 commits March 3, 2026 16:53

github-actions Bot added documentation Improvements or additions to documentation tests labels Mar 3, 2026

fix gradle check things

824e7f9

epugh reviewed Mar 6, 2026

View reviewed changes

github-actions Bot added client:solrj module:s3-repository labels Mar 11, 2026

github-actions Bot added cat:cloud cat:api labels Mar 16, 2026

github-actions Bot added the stale PR not updated in 60 days label May 18, 2026

github-actions Bot added the closed-stale Closed after being stale for 60 days label Jul 18, 2026

github-actions Bot closed this Jul 18, 2026

elangelo mentioned this pull request Oct 14, 2026

S3 backup repository: stop writing and depending on directory marker objects #5128

Open

6 of 8 tasks

Navigation Menu

Repository navigation

fix(backup): handle S3-style object stores in config restore - #4182

fix(backup): handle S3-style object stores in config restore#4182
elangelo wants to merge 6 commits into
apache:mainfrom
elangelo:handle-s3-restores-ignore-marker

elangelo commented Mar 3, 2026 •

edited

Loading

Uh oh!

epugh Mar 6, 2026

Uh oh!

elangelo Mar 6, 2026

Uh oh!

epugh Mar 6, 2026

Uh oh!

epugh Mar 6, 2026

Uh oh!

elangelo Mar 6, 2026

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

github-actions Bot commented Jul 18, 2026

Uh oh!

elangelo commented Oct 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

elangelo commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

epugh Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

elangelo Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

epugh Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

epugh Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

elangelo Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

github-actions Bot commented Jul 18, 2026

Uh oh!

elangelo commented Oct 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elangelo commented Mar 3, 2026 •

edited

Loading