You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "elangelo (via GitHub)" <gi...@apache.org> on 2026/10/14 08:41:53 UTC
[PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
elangelo opened a new pull request, #5128:
URL: https://github.com/apache/solr/pull/5128
Supersedes #4182 (closed as stale, and it can't be reopened after the branch was rebased). See that PR for the earlier review discussion.
# Description
The S3 backup repository simulated directories with zero-byte "directory marker" objects (keys ending in `/`, content type `application/x-directory`), and relied on them when restoring. Tools that copy buckets, such as `aws s3 sync`, only copy the real files and drop those markers. A backup copied that way could no longer be restored: the configset directory, among others, was reported as missing.
# Solution
Following the review on #4182, the S3 repository no longer uses directory markers at all, rather than adding a fallback for when they are missing:
- `S3StorageClient.createDirectory()` is a no-op. On S3 a directory is just a key prefix that has objects under it.
- `exists()` on a directory path and `isDirectory()` use a single prefix listing (`maxKeys=1`). This finds markerless directories as well as marker objects left by older Solr versions, so existing backups can still be restored. `deleteDirectory()` also removes those old markers.
- Writing a file no longer checks that its parent "directory" exists, and reading a file's length takes a single HEAD request again. Both mean fewer S3 requests.
- Core backup/restore code is unchanged and keeps calling `createDirectory()`/`exists()`. That keeps `LocalFileSystemRepository`, which needs real parent directories, and the other repositories working as before.
On S3 an empty directory can't exist any more. The shared `AbstractBackupRepositoryTest` contract assumed it could, so it gets a `supportsEmptyDirectories()` hook. S3 overrides it to run the shared tests instead of copying and overriding them.
Copilot (#4182) and Claude Code were used to develop this change.
# Tests
All new tests run against S3Mock instead of Mockito mocks:
- `S3IncrementalBackupTest#testRestoreBackupCopiedWithoutDirectoryMarkers` is the end-to-end regression test. It backs up a collection and checks the backup contains no marker objects. It then copies the backup file by file to another location, as `aws s3 sync` would, and restores it under a new configset name so the configset is uploaded from the backup. Finally it checks the configset and the document count.
- `S3BackupRepositoryTest#testLegacyDirectoryMarkers` checks that a backup containing old-style markers can still be checked for existence, listed, read and fully deleted.
- `S3PathsTest`, `S3ReadWriteTest` and `S3BackupRepositoryTest` are updated for markerless directories.
Ran `:solr:modules:s3-repository:test -Ptests.nightly=true`; all pass except `S3InstallShardTest#testInstallSkipsWriteLockFile`, which fails the same way on current `main` and is unrelated to this change. I also ran `LocalFileSystemRepositoryTest` and `GCSBackupRepositoryTest` to cover the shared contract test change, and `check -x test` on the changed modules.
# Checklist
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) 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.
- [ ] I have run `./gradlew check`.
- [x] I have added tests for my changes.
- [x] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
- [x] I have added a [changelog entry](https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc) for my change
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6315673563
okay, can you review my changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6305094900
Is this ready for final review and merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "elangelo (via GitHub)" <gi...@apache.org>.
elangelo commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6315723497
> okay, can you review my changes?
I was following pretty much live with your changes! Very nice cleanup!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6314668932
do you need this to go to 9x or is 10x fine?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "elangelo (via GitHub)" <gi...@apache.org>.
elangelo commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6310073996
> Is this ready for final review and merge?
This has been tested on my side and seems to have the intended behavior. Please go ahead and review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "elangelo (via GitHub)" <gi...@apache.org>.
elangelo commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6409935272
Thanks for the review, good catch.
Confirmed: with `incremental=false`, `BACKUPCORE` (and the v2 core backup API, where that is the default) failed on a fresh S3 prefix with "Directory to contain snapshots doesn't exist", because both `CreateCoreBackup` and `SnapShooter.validateCreateSnapshot()` required the location to exist.
Fixed in <commit>: both checks now only reject a missing location when `repository.supportsEmptyDirectories()` is true, so the local filesystem keeps its validation. `exists()` is still called first, so an unreachable bucket still fails up front. This also lets the ReplicationHandler `command=backup` write to a new S3 prefix.
Added `S3IncrementalBackupTest.testNonIncrementalCoreBackupToLocationThatDoesNotExistYet`: it runs `BACKUPCORE` with `incremental=false` to a fresh prefix, checks the snapshot landed in S3, then runs `RESTORECORE` and verifies the doc count. It fails without the fix and passes with it.
I have been applying for a JIRA account for quite a while now but all my requests seem to end in `/dev/null`.... hence no ticket :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6413596450
@elangelo argh... want to try again and then hit me up directly on email when you did? epugh@opensourceconnections.com
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "elangelo (via GitHub)" <gi...@apache.org>.
elangelo commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6314949413
> do you need this to go to 9x or is 10x fine?
9x would be wonderful. But we are looking into migrating to 10x ourselves.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "elangelo (via GitHub)" <gi...@apache.org>.
elangelo commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6341403564
I checked that build error and it seems to be part of a flaky test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "nick-boss-tech (via GitHub)" <gi...@apache.org>.
nick-boss-tech commented on PR #5128:
URL: https://github.com/apache/solr/pull/5128#issuecomment-6390579146
See the contributing guidelines, you need a ticket number at or near the top.
**One P2 regression found.**
Non-incremental core backups fail on a fresh S3 prefix. The patch makes createDirectory() a no-op, but CreateCoreBackup and SnapShooter still require that empty prefix to exist before writing. Previously, the directory marker satisfied those checks. [Changed behavior](https://github.com/apache/solr/blob/91500069e48cbdaec20d868cca04c1d042e2fda3/solr/modules/s3-repository/src/java/org/apache/solr/s3/S3StorageClient.java#L179-L185)
Both remaining checks need to respect supportsEmptyDirectories(), while preserving local-filesystem validation. Add an S3Mock test using BACKUPCORE with incremental=false and a fresh prefix. The new integration tests cover incremental collection backups.
The markerless restore approach otherwise looks coherent and addresses the earlier review requests.
AI-generated, static review.
Flaky test may be fixed with this ticket/PR:
https://issues.apache.org/jira/browse/SOLR-18506
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] S3 backup repository: stop writing and depending on directory marker objects [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #5128:
URL: https://github.com/apache/solr/pull/5128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org