Skip to content

Commit

Permalink
remote: Do not retry bytestream read requests if the whole file was d…
Browse files Browse the repository at this point in the history
…ownloaded.

Previously, an error at the end of the Read stream would result in a pointless retry request for 0 bytes of data.
  • Loading branch information
benjaminp committed Aug 5, 2021
1 parent 9d0d971 commit e994003
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,14 @@ public void onNext(ReadResponse readResponse) {

@Override
public void onError(Throwable t) {
if (offset.get() == digest.getSizeBytes()) {
// If the file was fully downloaded, it doesn't matter if there was an error at
// the end of the stream.
logger.atInfo().withCause(t).log(
"ignoring error because file was fully received");
onCompleted();
return;
}
Status status = Status.fromThrowable(t);
if (status.getCode() == Status.Code.NOT_FOUND) {
future.setException(new CacheNotFoundException(digest));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,28 @@ public void read(ReadRequest request, StreamObserver<ReadResponse> responseObser
Mockito.verify(mockBackoff, Mockito.never()).nextDelayMillis(any(Exception.class));
}

@Test
public void downloadBlobDoesNotRetryZeroLengthRequests()
throws IOException, InterruptedException {
Backoff mockBackoff = Mockito.mock(Backoff.class);
final GrpcCacheClient client =
newClient(Options.getDefaults(RemoteOptions.class), () -> mockBackoff);
final Digest digest = DIGEST_UTIL.computeAsUtf8("abcdefg");
serviceRegistry.addService(
new ByteStreamImplBase() {
@Override
public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) {
assertThat(request.getResourceName().contains(digest.getHash())).isTrue();
assertThat(request.getReadOffset()).isEqualTo(0);
ByteString data = ByteString.copyFromUtf8("abcdefg");
responseObserver.onNext(ReadResponse.newBuilder().setData(data).build());
responseObserver.onError(Status.INTERNAL.asException());
}
});
assertThat(new String(downloadBlob(context, client, digest), UTF_8)).isEqualTo("abcdefg");
Mockito.verify(mockBackoff, Mockito.never()).nextDelayMillis(any(Exception.class));
}

@Test
public void downloadBlobPassesThroughDeadlineExceededWithoutProgress() throws IOException {
Backoff mockBackoff = Mockito.mock(Backoff.class);
Expand Down

0 comments on commit e994003

Please sign in to comment.