You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "iprithv (via GitHub)" <gi...@apache.org> on 2026/08/12 14:29:31 UTC
[PR] GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
iprithv opened a new pull request, #4726:
URL: https://github.com/apache/solr/pull/4726
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.
--
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] GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3767474782
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
+ return (ReadChannel)
+ Proxy.newProxyInstance(
+ ReadChannel.class.getClassLoader(),
+ new Class<?>[] {ReadChannel.class},
+ new InvocationHandler() {
+ private boolean returnedZero = false;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if ("read".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof ByteBuffer) {
+ if (!returnedZero) {
+ returnedZero = true;
+ return 0;
+ }
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ }
+ });
+ }
+
+ private static Object invokeAndUnwrap(Method method, Object target, Object[] args)
Review 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?
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3790342879
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
+ return (ReadChannel)
+ Proxy.newProxyInstance(
+ ReadChannel.class.getClassLoader(),
+ new Class<?>[] {ReadChannel.class},
+ new InvocationHandler() {
+ private boolean returnedZero = false;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if ("read".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof ByteBuffer) {
+ if (!returnedZero) {
+ returnedZero = true;
+ return 0;
+ }
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ }
+ });
+ }
+
+ private static Object invokeAndUnwrap(Method method, Object target, Object[] args)
+ throws Throwable {
+ try {
+ return method.invoke(target, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ }
+
+ private GCSBackupRepository createRepositoryWithStorage(Storage storage) {
+ TestGCSBackupRepository repo = new TestGCSBackupRepository(storage);
+ repo.init(getBaseBackupRepositoryConfiguration());
+ return repo;
+ }
+
+ private static class TestGCSBackupRepository extends GCSBackupRepository {
Review Comment:
sure, added a short javadoc here. thanks!
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4726:
URL: https://github.com/apache/solr/pull/4726#issuecomment-5268397495
@iprithv there is a template for prs, and I tweaked the title and added the link to jira in the PR ;-).
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4726:
URL: https://github.com/apache/solr/pull/4726#issuecomment-5344132330
Okay, this all makes sense to me. The part that I can't test is the actual use of this against a live GCS.... Not sure who CAN do that.. I'll leave this open for antoher few days and then 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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3790344651
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
Review Comment:
sure, added a javadoc for this one too. thanks!
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3767565305
##########
changelog/unreleased/gcs-copyIndexFileTo-fix.yml:
##########
@@ -0,0 +1,5 @@
+title: >
+ Fixed GCS backup restores silently swallowing failures and stopping early on zero-byte reads (SOLR-18250).
+type: fixed
+authors:
+ - name: Prithvi S
Review Comment:
Changelog fragments are expected to include a `links` section (per dev-docs/changelog.adoc) so the entry can be traced back to the relevant JIRA/PR. This fragment currently omits `links`.
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3790344009
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
+ return (ReadChannel)
+ Proxy.newProxyInstance(
+ ReadChannel.class.getClassLoader(),
+ new Class<?>[] {ReadChannel.class},
+ new InvocationHandler() {
+ private boolean returnedZero = false;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if ("read".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof ByteBuffer) {
+ if (!returnedZero) {
+ returnedZero = true;
+ return 0;
+ }
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ }
+ });
+ }
+
+ private static Object invokeAndUnwrap(Method method, Object target, Object[] args)
Review Comment:
agreed it looks a bit heavy. added a note :)
--
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] GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3767467561
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
+ return (ReadChannel)
+ Proxy.newProxyInstance(
+ ReadChannel.class.getClassLoader(),
+ new Class<?>[] {ReadChannel.class},
+ new InvocationHandler() {
+ private boolean returnedZero = false;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
+ if ("read".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof ByteBuffer) {
+ if (!returnedZero) {
+ returnedZero = true;
+ return 0;
+ }
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ }
+ });
+ }
+
+ private static Object invokeAndUnwrap(Method method, Object target, Object[] args)
+ throws Throwable {
+ try {
+ return method.invoke(target, args);
+ } catch (InvocationTargetException e) {
+ throw e.getCause();
+ }
+ }
+
+ private GCSBackupRepository createRepositoryWithStorage(Storage storage) {
+ TestGCSBackupRepository repo = new TestGCSBackupRepository(storage);
+ repo.init(getBaseBackupRepositoryConfiguration());
+ return repo;
+ }
+
+ private static class TestGCSBackupRepository extends GCSBackupRepository {
Review Comment:
maybe just a bit of docs to cue the user that we are udisng this to be able to simulate certain behaviors?
--
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] GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on code in PR #4726:
URL: https://github.com/apache/solr/pull/4726#discussion_r3767477926
##########
solr/modules/gcs-repository/src/test/org/apache/solr/gcs/GCSBackupRepositoryTest.java:
##########
@@ -77,4 +93,157 @@ public void testInitStoreDoesNotFailWithMissingCredentials() {
gcsBackupRepository.init(new NamedList<>(config));
}
+
+ @Test
+ public void testCopyIndexFileToPropagatesReadFailures() throws Exception {
+ Storage failingStorage = createFailingStorage();
+ GCSBackupRepository repo = createRepositoryWithStorage(failingStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ IOException thrown =
+ expectThrows(
+ IOException.class,
+ () -> repo.copyIndexFileTo(sourceDir, "any.dat", dest, "dest.dat"));
+ assertTrue(thrown.getMessage().contains("Failed to copy index file from GCS"));
+ assertNotNull(thrown.getCause());
+ assertTrue(thrown.getCause() instanceof StorageException);
+ assertEquals("simulated GCS read failure", thrown.getCause().getMessage());
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToHandlesZeroByteReads() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ Storage zeroReturningStorage = createZeroReturningStorage(realStorage);
+ GCSBackupRepository proxyRepo = createRepositoryWithStorage(zeroReturningStorage);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ proxyRepo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ @Test
+ public void testCopyIndexFileToCopiesFile() throws Exception {
+ Storage realStorage = LocalStorageHelper.customOptions(false).getService();
+ byte[] data = new byte[100];
+ random().nextBytes(data);
+ // "solrBackupsBucket" matches GCSConfigParser.DEFAULT_GCS_BUCKET_VALUE
+ String bucketName = "solrBackupsBucket";
+
+ GCSBackupRepository repo = createRepositoryWithStorage(realStorage);
+ URI sourceDir = repo.resolve(getBaseUri(), "backup");
+ BlobId blobId = BlobId.of(bucketName, sourceDir + "/source.dat");
+ realStorage.create(BlobInfo.newBuilder(blobId).build(), data);
+
+ try (Directory dest = new ByteBuffersDirectory()) {
+ repo.copyIndexFileTo(sourceDir, "source.dat", dest, "dest.dat");
+ try (IndexInput in = dest.openInput("dest.dat", IOContext.DEFAULT)) {
+ assertEquals(data.length, in.length());
+ byte[] read = new byte[data.length];
+ in.readBytes(read, 0, data.length);
+ assertArrayEquals(data, read);
+ }
+ }
+ }
+
+ private static Storage createFailingStorage() {
+ Storage delegate = LocalStorageHelper.customOptions(false).getService();
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())) {
+ throw new StorageException(0, "simulated GCS read failure");
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static Storage createZeroReturningStorage(Storage delegate) {
+ return (Storage)
+ Proxy.newProxyInstance(
+ Storage.class.getClassLoader(),
+ new Class<?>[] {Storage.class},
+ (proxy, method, args) -> {
+ if ("reader".equals(method.getName())
+ && args != null
+ && args.length == 1
+ && args[0] instanceof BlobId) {
+ ReadChannel realChannel = (ReadChannel) invokeAndUnwrap(method, delegate, args);
+ return createZeroFirstReadChannel(realChannel);
+ }
+ return invokeAndUnwrap(method, delegate, args);
+ });
+ }
+
+ private static ReadChannel createZeroFirstReadChannel(ReadChannel delegate) {
Review Comment:
likewise a doc, and I think it's cool how you are setting up specific scenarios.
--
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] GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "iprithv (via GitHub)" <gi...@apache.org>.
iprithv commented on PR #4726:
URL: https://github.com/apache/solr/pull/4726#issuecomment-5268245021
https://issues.apache.org/jira/browse/SOLR-18250
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh commented on PR #4726:
URL: https://github.com/apache/solr/pull/4726#issuecomment-5268410734
i ran the ci/cd jobs, can you review.. this seems reasonable fix direction with tests!
--
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] SOLR-18250: GCSBackupRepository.copyIndexFileTo must not swallow failures [solr]
Posted by "epugh (via GitHub)" <gi...@apache.org>.
epugh merged PR #4726:
URL: https://github.com/apache/solr/pull/4726
--
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