Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Error handling for Downloads #2043

Merged
merged 7 commits into from
Jun 6, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -86,17 +86,6 @@ public static Function<WriteChannel, Optional<BlobInfo>> maybeGetBlobInfoFunctio
};
}

public static Function<WriteChannel, Optional<BlobInfo>> maybeGetBlobInfoFunction() {
sydney-munro marked this conversation as resolved.
Show resolved Hide resolved
return writeChannel -> {
Optional<StorageObject> so = maybeGetStorageObjectFunction().apply(writeChannel);
if (so.isPresent()) {
return Optional.of(Conversions.apiary().blobInfo().decode(so.get()));
} else {
return Optional.empty();
}
};
}

public static ApiFuture<BlobInfo> getBlobInfoFromReadChannelFunction(ReadChannel c) {
if (c instanceof StorageReadChannel) {
StorageReadChannel src = (StorageReadChannel) c;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@
package com.google.cloud.storage.transfermanager;

import com.google.cloud.ReadChannel;
import com.google.cloud.storage.BlobId;
import com.google.cloud.storage.BlobInfo;
import com.google.cloud.storage.Storage;
import com.google.cloud.storage.Storage.BlobSourceOption;
import com.google.cloud.storage.StorageException;
import com.google.common.io.ByteStreams;
import java.io.IOException;
import java.nio.channels.FileChannel;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
Expand Down Expand Up @@ -50,16 +49,26 @@ final class DirectDownloadCallable implements Callable<DownloadResult> {
@Override
public DownloadResult call() {
Path path = TransferManagerUtils.createDestPath(parallelDownloadConfig, originalBlob);
try (ReadChannel rc = storage.reader(originalBlob.getBlobId(), opts)) {
long bytesCopied = -1L;
try (ReadChannel rc =
storage.reader(
BlobId.of(parallelDownloadConfig.getBucketName(), originalBlob.getName()), opts)) {
FileChannel wc =
FileChannel.open(
path,
StandardOpenOption.WRITE,
StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
ByteStreams.copy(rc, wc);
} catch (IOException e) {
throw new StorageException(e);
bytesCopied = ByteStreams.copy(rc, wc);
} catch (Exception e) {
if (bytesCopied == -1) {
return DownloadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_START)
.setException(e)
.build();
}
return DownloadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_FINISH)
.setException(e)
.build();
}
DownloadResult result =
DownloadResult.newBuilder(originalBlob, TransferStatus.SUCCESS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import static com.google.common.base.Preconditions.checkState;

import com.google.cloud.storage.BlobInfo;
import com.google.cloud.storage.StorageException;
import com.google.common.base.MoreObjects;
import java.nio.file.Path;
import java.util.Comparator;
Expand All @@ -35,13 +34,13 @@ public final class DownloadResult {
@NonNull private final BlobInfo input;
@MonotonicNonNull private final Path outputDestination;
@NonNull private final TransferStatus status;
@MonotonicNonNull private final StorageException exception;
@MonotonicNonNull private final Exception exception;

private DownloadResult(
@NonNull BlobInfo input,
Path outputDestination,
@NonNull TransferStatus status,
StorageException exception) {
Exception exception) {
this.input = input;
this.outputDestination = outputDestination;
this.status = status;
Expand All @@ -64,7 +63,7 @@ private DownloadResult(
return status;
}

public @NonNull StorageException getException() {
public @NonNull Exception getException() {
checkState(
status == TransferStatus.FAILED_TO_FINISH || status == TransferStatus.FAILED_TO_START,
"getException() is only valid when an unexpected error has occurred but status was %s",
Expand Down Expand Up @@ -111,7 +110,7 @@ public static final class Builder {
private @NonNull BlobInfo input;
private @MonotonicNonNull Path outputDestination;
private @NonNull TransferStatus status;
private @MonotonicNonNull StorageException exception;
private @MonotonicNonNull Exception exception;

private Builder(@NonNull BlobInfo input, @NonNull TransferStatus status) {
this.input = input;
Expand All @@ -133,7 +132,7 @@ public Builder setStatus(@NonNull TransferStatus status) {
return this;
}

public Builder setException(@NonNull StorageException exception) {
public Builder setException(@NonNull Exception exception) {
this.exception = exception;
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public UploadResult call() throws Exception {
}

private UploadResult uploadWithoutChunking() {
long bytesCopied = 0L;
long bytesCopied = -1L;
try {
Optional<BlobInfo> newBlob;
try (FileChannel r = FileChannel.open(sourceFile, StandardOpenOption.READ);
Expand All @@ -74,7 +74,7 @@ private UploadResult uploadWithoutChunking() {
.setUploadedBlob(newBlob.get())
.build();
} catch (Exception e) {
if (bytesCopied == 0) {
if (bytesCopied == -1) {
return UploadResult.newBuilder(originalBlob, TransferStatus.FAILED_TO_START)
.setException(e)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,49 @@ public void uploadNonexistentFile() throws Exception {
}
}

@Test
public void downloadNonexistentBucket() throws Exception {
TransferManagerConfig config =
TransferManagerConfigTestingInstances.defaults(storage.getOptions());
try (TransferManager transferManager = config.getService()) {
String bucketName = bucket.getName() + "-does-not-exist";
ParallelDownloadConfig parallelDownloadConfig =
ParallelDownloadConfig.newBuilder()
.setBucketName(bucketName)
.setDownloadDirectory(baseDir)
.build();
DownloadJob job = transferManager.downloadBlobs(blobs, parallelDownloadConfig);
List<DownloadResult> downloadResults = ApiFutures.allAsList(job.getDownloadResults()).get();
assertThat(downloadResults.get(0).getStatus()).isEqualTo(TransferStatus.FAILED_TO_START);
}
}

@Test
public void downloadBlobsChunkedFail() throws Exception {
TransferManagerConfig config =
TransferManagerConfigTestingInstances.defaults(storage.getOptions())
.toBuilder()
.setAllowDivideAndConquer(true)
.setPerWorkerBufferSize(128 * 1024)
.build();
try (TransferManager transferManager = config.getService()) {
String bucketName = bucket.getName() + "-does-not-exist";
ParallelDownloadConfig parallelDownloadConfig =
ParallelDownloadConfig.newBuilder()
.setBucketName(bucketName)
.setDownloadDirectory(baseDir)
.build();
DownloadJob job = transferManager.downloadBlobs(blobs, parallelDownloadConfig);
List<DownloadResult> downloadResults = ApiFutures.allAsList(job.getDownloadResults()).get();
assertThat(downloadResults).hasSize(3);
try {
assertThat(downloadResults.get(0).getStatus()).isEqualTo(TransferStatus.FAILED_TO_START);
BenWhitehead marked this conversation as resolved.
Show resolved Hide resolved
} finally {
cleanUpFiles(downloadResults);
}
}
}

private void cleanUpFiles(List<DownloadResult> results) throws IOException {
// Cleanup downloaded blobs and the parent directory
for (DownloadResult res : results) {
Expand Down