SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures - #4726
SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures#4726iprithv wants to merge 1 commit into
Conversation
Signed-off-by: prithvi <prithvisivasankar@gmail.com>
| return repo; | ||
| } | ||
|
|
||
| private static class TestGCSBackupRepository extends GCSBackupRepository { |
There was a problem hiding this comment.
maybe just a bit of docs to cue the user that we are udisng this to be able to simulate certain behaviors?
| }); | ||
| } | ||
|
|
||
| private static Object invokeAndUnwrap(Method method, Object target, Object[] args) |
There was a problem hiding this comment.
huh.. everytime I see these very advanced java method manipulation, I get slightly nervous, but maybe just a quick doc that says what we are using this for?
| }); | ||
| } | ||
|
|
||
| private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) { |
There was a problem hiding this comment.
likewise a doc, and I think it's cool how you are setting up specific scenarios.
epugh
changed the title
|
@iprithv there is a template for prs, and I tweaked the title and added the link to jira in the PR ;-). that allows linking to jira ;-) |
|
i ran the ci/cd jobs, can you review.. this seems reasonable fix direction with tests! |
There was a problem hiding this comment.
Pull request overview
This PR fixes robustness and observability issues in the GCS backup restore path by ensuring GCSBackupRepository.copyIndexFileTo neither truncates output on zero-byte reads nor hides underlying failures, aligning behavior with the S3 repository’s semantics.
Changes:
- Fix
copyIndexFileToread loop to useread(...) != -1so0is not treated as EOF, preventing truncated restores. - Stop swallowing exceptions: log failures at error level and propagate
IOExceptions; wrap unexpected runtime failures in anIOExceptionwith context. - Add/extend unit tests covering read failures, zero-byte reads, and a successful copy path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| solr/modules/gcs-repository/src/java/org/apache/solr/gcs/GCSBackupRepository.java | Fixes the copy loop EOF condition and ensures failures are propagated instead of being swallowed. |
| solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java | Adds tests validating failure propagation and correct handling of zero-byte reads during copy. |
| changelog/unreleased/gcs-copyIndexFileTo-fix.yml | Adds a changelog entry describing the fix (needs links to match project conventions). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| title: > | ||
| Fixed GCS backup restores silently swallowing failures and stopping early on zero-byte reads (SOLR-18250). | ||
| type: fixed | ||
| authors: | ||
| - name: Prithvi S |
https://issues.apache.org/jira/browse/SOLR-18250
GCSBackupRepository.copyIndexFileTo currently:
Swallows all exceptions via a bare catch (Exception e) { log.info("Here's an exception e", e); }, so a failed or incomplete restore can look successful.
Uses while (readChannel.read(buffer) > 0), which is incorrect per ReadableByteChannel.read: a 0 return is not EOF, so the loop can terminate early and leave a truncated destination file. The S3 equivalent already uses != -1.