Skip to content

Commit 961f3cb

Browse files
BenWhiteheadBenWhitehead
committed
chore: refactor RetryContext#recordError to return the throwable provided to it instead of blindly wrapping everything in a CancelledException
Add Hasher#hash(ByteString) and Hasher#validateUnchecked(ByteString) that will throw an unchecked exception instead of a checked IOException. When a retry budget is exhaused a supressed execption with a comment will be included with comment details. A multi attempt failure will now look like: ``` com.google.api.gax.rpc.ResourceExhaustedException: {resource exhausted} at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:60) at com.google.cloud.storage.RetryContextTest.apiException(RetryContextTest.java:186) at com.google.cloud.storage.RetryContextTest.retryable_cancelledException_when_maxAttemptBudget_multipleAttempts_previousErrorsIncludedAsSuppressed(RetryContextTest.java:70) ... test runner frames Suppressed: com.google.cloud.storage.RetryContext$RetryBudgetExhaustedComment: Operation failed to complete within retry limit (attempts: 3, maxAttempts: 3) previous failures follow in order of occurrence Suppressed: com.google.api.gax.rpc.UnavailableException: {unavailable} at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:72) at com.google.cloud.storage.RetryContextTest.apiException(RetryContextTest.java:186) at com.google.cloud.storage.RetryContextTest.retryable_cancelledException_when_maxAttemptBudget_multipleAttempts_previousErrorsIncludedAsSuppressed(RetryContextTest.java:68) ... 27 more Suppressed: com.google.api.gax.rpc.InternalException: {internal} at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:70) at com.google.cloud.storage.RetryContextTest.apiException(RetryContextTest.java:186) at com.google.cloud.storage.RetryContextTest.retryable_cancelledException_when_maxAttemptBudget_multipleAttempts_previousErrorsIncludedAsSuppressed(RetryContextTest.java:69) ... 27 more ``` A single attempt failure will now look like: ``` com.google.api.gax.rpc.UnavailableException: {unavailable} at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:72) at com.google.cloud.storage.RetryContextTest.apiException(RetryContextTest.java:186) at com.google.cloud.storage.RetryContextTest.nonretryable_cancelledException_regardlessOfAttemptBudget(RetryContextTest.java:94) ... test runner frames Suppressed: com.google.cloud.storage.RetryContext$RetryBudgetExhaustedComment: Unretryable error (attempts: 1, maxAttempts: 3) ```
1 parent 98dbb02 commit 961f3cb

File tree

6 files changed

+207
-69
lines changed

6 files changed

+207
-69
lines changed

‎google-cloud-storage/src/main/java/com/google/cloud/storage/BlobDescriptorStream.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.api.gax.rpc.ResponseObserver;
2727
import com.google.api.gax.rpc.StreamController;
2828
import com.google.cloud.storage.GrpcUtils.ZeroCopyBidiStreamingCallable;
29+
import com.google.cloud.storage.Hasher.UncheckedChecksumMismatchException;
2930
import com.google.cloud.storage.ResponseContentLifecycleHandle.ChildRef;
3031
import com.google.cloud.storage.RetryContext.OnSuccess;
3132
import com.google.common.base.Preconditions;
@@ -261,8 +262,8 @@ public void onResponse(BidiReadObjectResponse response) {
261262
try {
262263
// todo: benchmark how long it takes to compute this checksum and whether it needs to
263264
// happen on a non-io thread
264-
Hasher.enabled().validate(Crc32cValue.of(crc32C), content);
265-
} catch (IOException e) {
265+
Hasher.enabled().validateUnchecked(Crc32cValue.of(crc32C), content);
266+
} catch (UncheckedChecksumMismatchException e) {
266267
read.recordError(e, restartReadFromCurrentOffset(id), read::unsafeFail);
267268
continue;
268269
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageImpl.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
import com.google.cloud.storage.Conversions.Decoder;
5555
import com.google.cloud.storage.GrpcUtils.ZeroCopyBidiStreamingCallable;
5656
import com.google.cloud.storage.GrpcUtils.ZeroCopyServerStreamingCallable;
57-
import com.google.cloud.storage.Hasher.ChecksumMismatchException;
57+
import com.google.cloud.storage.Hasher.UncheckedChecksumMismatchException;
5858
import com.google.cloud.storage.HmacKey.HmacKeyMetadata;
5959
import com.google.cloud.storage.HmacKey.HmacKeyState;
6060
import com.google.cloud.storage.PostPolicyV4.PostConditionsV4;
@@ -229,7 +229,7 @@ final class GrpcStorageImpl extends BaseService<StorageOptions>
229229
@Override
230230
public boolean shouldRetry(Throwable previousThrowable, Object previousResponse) {
231231
// this is only retryable with read object range, not other requests
232-
return previousThrowable instanceof ChecksumMismatchException
232+
return previousThrowable instanceof UncheckedChecksumMismatchException
233233
|| previousThrowable instanceof OutOfRangeException
234234
|| retryAlgorithmManager.idempotent().shouldRetry(previousThrowable, null);
235235
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/Hasher.java‎

Lines changed: 77 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,19 @@
1616

1717
package com.google.cloud.storage;
1818

19+
import com.google.api.gax.grpc.GrpcStatusCode;
20+
import com.google.api.gax.rpc.DataLossException;
1921
import com.google.cloud.storage.Crc32cValue.Crc32cLengthKnown;
2022
import com.google.common.hash.Hashing;
2123
import com.google.protobuf.ByteString;
24+
import io.grpc.Status.Code;
2225
import java.io.IOException;
2326
import java.nio.ByteBuffer;
2427
import java.util.List;
2528
import java.util.Locale;
2629
import java.util.function.Supplier;
2730
import javax.annotation.concurrent.Immutable;
31+
import org.checkerframework.checker.nullness.qual.NonNull;
2832
import org.checkerframework.checker.nullness.qual.Nullable;
2933

3034
interface Hasher {
@@ -37,9 +41,15 @@ default Crc32cLengthKnown hash(Supplier<ByteBuffer> b) {
3741
@Nullable
3842
Crc32cLengthKnown hash(ByteBuffer b);
3943

40-
void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) throws IOException;
44+
@Nullable
45+
Crc32cLengthKnown hash(ByteString byteString);
46+
47+
void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) throws ChecksumMismatchException;
48+
49+
void validate(Crc32cValue<?> expected, ByteString byteString) throws ChecksumMismatchException;
4150

42-
void validate(Crc32cValue<?> expected, ByteString byteString) throws IOException;
51+
void validateUnchecked(Crc32cValue<?> expected, ByteString byteString)
52+
throws UncheckedChecksumMismatchException;
4353

4454
@Nullable
4555
Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2);
@@ -63,12 +73,20 @@ public Crc32cLengthKnown hash(ByteBuffer b) {
6373
return null;
6474
}
6575

76+
@Override
77+
public @Nullable Crc32cLengthKnown hash(ByteString byteString) {
78+
return null;
79+
}
80+
6681
@Override
6782
public void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) {}
6883

6984
@Override
7085
public void validate(Crc32cValue<?> expected, ByteString b) {}
7186

87+
@Override
88+
public void validateUnchecked(Crc32cValue<?> expected, ByteString byteString) {}
89+
7290
@Override
7391
public @Nullable Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2) {
7492
return null;
@@ -82,35 +100,56 @@ class GuavaHasher implements Hasher {
82100
private GuavaHasher() {}
83101

84102
@Override
85-
public Crc32cLengthKnown hash(ByteBuffer b) {
103+
public @NonNull Crc32cLengthKnown hash(Supplier<ByteBuffer> b) {
104+
return hash(b.get());
105+
}
106+
107+
@Override
108+
public @NonNull Crc32cLengthKnown hash(ByteBuffer b) {
86109
int remaining = b.remaining();
87110
return Crc32cValue.of(Hashing.crc32c().hashBytes(b).asInt(), remaining);
88111
}
89112

90113
@SuppressWarnings({"ConstantConditions", "UnstableApiUsage"})
91114
@Override
92-
public void validate(Crc32cValue<?> expected, ByteString byteString) throws IOException {
93-
List<ByteBuffer> b = byteString.asReadOnlyByteBufferList();
94-
long remaining = 0;
115+
public @NonNull Crc32cLengthKnown hash(ByteString byteString) {
116+
List<ByteBuffer> buffers = byteString.asReadOnlyByteBufferList();
95117
com.google.common.hash.Hasher crc32c = Hashing.crc32c().newHasher();
96-
for (ByteBuffer tmp : b) {
97-
remaining += tmp.remaining();
98-
crc32c.putBytes(tmp);
118+
for (ByteBuffer b : buffers) {
119+
crc32c.putBytes(b);
99120
}
100-
Crc32cLengthKnown actual = Crc32cValue.of(crc32c.hash().asInt(), remaining);
121+
return Crc32cValue.of(crc32c.hash().asInt(), byteString.size());
122+
}
123+
124+
@SuppressWarnings({"ConstantConditions"})
125+
@Override
126+
public void validate(Crc32cValue<?> expected, ByteString byteString)
127+
throws ChecksumMismatchException {
128+
Crc32cLengthKnown actual = hash(byteString);
101129
if (!actual.eqValue(expected)) {
102130
throw new ChecksumMismatchException(expected, actual);
103131
}
104132
}
105133

106134
@Override
107-
public void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b) throws IOException {
108-
Crc32cLengthKnown actual = hash(b);
135+
public void validate(Crc32cValue<?> expected, Supplier<ByteBuffer> b)
136+
throws ChecksumMismatchException {
137+
@NonNull Crc32cLengthKnown actual = hash(b);
109138
if (!actual.eqValue(expected)) {
110139
throw new ChecksumMismatchException(expected, actual);
111140
}
112141
}
113142

143+
@SuppressWarnings({"ConstantConditions"})
144+
@Override
145+
public void validateUnchecked(Crc32cValue<?> expected, ByteString byteString)
146+
throws UncheckedChecksumMismatchException {
147+
Crc32cLengthKnown actual = hash(byteString);
148+
if (!actual.eqValue(expected)) {
149+
throw new UncheckedChecksumMismatchException(expected, actual);
150+
}
151+
}
152+
114153
@Override
115154
@Nullable
116155
public Crc32cLengthKnown nullSafeConcat(Crc32cLengthKnown r1, Crc32cLengthKnown r2) {
@@ -145,4 +184,30 @@ Crc32cValue<?> getActual() {
145184
return actual;
146185
}
147186
}
187+
188+
final class UncheckedChecksumMismatchException extends DataLossException {
189+
private static final GrpcStatusCode STATUS_CODE = GrpcStatusCode.of(Code.DATA_LOSS);
190+
private final Crc32cValue<?> expected;
191+
private final Crc32cLengthKnown actual;
192+
193+
private UncheckedChecksumMismatchException(Crc32cValue<?> expected, Crc32cLengthKnown actual) {
194+
super(
195+
String.format(
196+
"Mismatch checksum value. Expected %s actual %s",
197+
expected.debugString(), actual.debugString()),
198+
/*cause=*/ null,
199+
STATUS_CODE,
200+
/*retryable=*/ false);
201+
this.expected = expected;
202+
this.actual = actual;
203+
}
204+
205+
Crc32cValue<?> getExpected() {
206+
return expected;
207+
}
208+
209+
Crc32cLengthKnown getActual() {
210+
return actual;
211+
}
212+
}
148213
}

‎google-cloud-storage/src/main/java/com/google/cloud/storage/RetryContext.java‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,18 +16,13 @@
1616

1717
package com.google.cloud.storage;
1818

19-
import com.google.api.gax.grpc.GrpcStatusCode;
2019
import com.google.api.gax.retrying.ResultRetryAlgorithm;
21-
import com.google.api.gax.rpc.ApiException;
22-
import com.google.api.gax.rpc.ApiExceptionFactory;
2320
import com.google.cloud.storage.Retrying.RetryingDependencies;
24-
import io.grpc.Status.Code;
2521
import java.util.LinkedList;
2622
import java.util.List;
2723

2824
final class RetryContext {
2925

30-
private static final GrpcStatusCode CANCELLED = GrpcStatusCode.of(Code.CANCELLED);
3126
private final RetryingDependencies retryingDependencies;
3227
private final ResultRetryAlgorithm<?> algorithm;
3328
private List<Throwable> failures;
@@ -42,7 +37,7 @@ void reset() {
4237
failures = new LinkedList<>();
4338
}
4439

45-
public void recordError(Throwable t, OnSuccess onSuccess, OnFailure onFailure) {
40+
public <T extends Throwable> void recordError(T t, OnSuccess onSuccess, OnFailure<T> onFailure) {
4641
int failureCount = failures.size() + 1 /* include t in the count*/;
4742
int maxAttempts = retryingDependencies.getRetrySettings().getMaxAttempts();
4843
boolean shouldRetry = algorithm.shouldRetry(t, null);
@@ -58,12 +53,17 @@ public void recordError(Throwable t, OnSuccess onSuccess, OnFailure onFailure) {
5853
onSuccess.onSuccess();
5954
} else {
6055
String msg =
61-
String.format("%s (attempts: %d, maxAttempts: %d)", msgPrefix, failureCount, maxAttempts);
62-
ApiException cancelled = ApiExceptionFactory.createException(msg, t, CANCELLED, false);
56+
String.format(
57+
"%s (attempts: %d, maxAttempts: %d)%s",
58+
msgPrefix,
59+
failureCount,
60+
maxAttempts,
61+
failures.isEmpty() ? "" : " previous failures follow in order of occurrence");
62+
t.addSuppressed(new RetryBudgetExhaustedComment(msg));
6363
for (Throwable failure : failures) {
64-
cancelled.addSuppressed(failure);
64+
t.addSuppressed(failure);
6565
}
66-
onFailure.onFailure(cancelled);
66+
onFailure.onFailure(t);
6767
}
6868
}
6969

@@ -93,7 +93,21 @@ interface OnSuccess {
9393
}
9494

9595
@FunctionalInterface
96-
interface OnFailure {
97-
void onFailure(Throwable t);
96+
interface OnFailure<T extends Throwable> {
97+
void onFailure(T t);
98+
}
99+
100+
/**
101+
* Define a custom exception which can carry a comment about the budget exhaustion, so we can
102+
* include it as a suppressed exception, but don't fill in any stack frames. This is a throwable
103+
* only because it is the only way we can include it into an exception that will by default print
104+
* with the exception stacktrace.
105+
*
106+
* @see Throwable#addSuppressed(Throwable)
107+
*/
108+
private static final class RetryBudgetExhaustedComment extends Throwable {
109+
private RetryBudgetExhaustedComment(String comment) {
110+
super(comment, /*cause=*/ null, /*enableSuppression=*/ true, /*writableStackTrace=*/ false);
111+
}
98112
}
99113
}

‎google-cloud-storage/src/test/java/com/google/cloud/storage/ITBlobDescriptorFakeTest.java‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,11 @@
2626
import com.google.api.gax.retrying.RetrySettings;
2727
import com.google.api.gax.rpc.AbortedException;
2828
import com.google.api.gax.rpc.ApiExceptions;
29-
import com.google.api.gax.rpc.CancelledException;
29+
import com.google.api.gax.rpc.DataLossException;
3030
import com.google.api.gax.rpc.OutOfRangeException;
3131
import com.google.api.gax.rpc.UnavailableException;
3232
import com.google.cloud.storage.Crc32cValue.Crc32cLengthKnown;
33-
import com.google.cloud.storage.Hasher.ChecksumMismatchException;
33+
import com.google.cloud.storage.Hasher.UncheckedChecksumMismatchException;
3434
import com.google.cloud.storage.it.ChecksummedTestContent;
3535
import com.google.cloud.storage.it.GrpcPlainRequestLoggingInterceptor;
3636
import com.google.common.collect.ImmutableMap;
@@ -422,7 +422,7 @@ public void bidiReadObjectError() throws Exception {
422422
}
423423

424424
@Test
425-
public void objectRangeData_checksumFailure() throws Exception {
425+
public void expectRetryForRangeWithFailedChecksumValidation() throws Exception {
426426

427427
ChecksummedTestContent expected = ChecksummedTestContent.of(ALL_OBJECT_BYTES, 10, 20);
428428

@@ -623,16 +623,17 @@ public void onNext(BidiReadObjectRequest request) {
623623
try (BlobDescriptor bd = futureBlobDescriptor.get(5, TimeUnit.SECONDS)) {
624624
ApiFuture<byte[]> future = bd.readRangeAsBytes(RangeSpec.of(10, 10));
625625

626-
CancelledException cancelledException =
626+
DataLossException dataLossException =
627627
assertThrows(
628-
CancelledException.class, () -> ApiExceptions.callAndTranslateApiException(future));
628+
DataLossException.class, () -> ApiExceptions.callAndTranslateApiException(future));
629629

630-
assertThat(cancelledException).hasCauseThat().isInstanceOf(ChecksumMismatchException.class);
631-
Throwable[] suppressed = cancelledException.getSuppressed();
630+
assertThat(dataLossException).isInstanceOf(UncheckedChecksumMismatchException.class);
631+
Throwable[] suppressed = dataLossException.getSuppressed();
632632
List<String> suppressedMessages =
633633
Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
634634
assertThat(suppressedMessages)
635635
.containsExactly(
636+
"Operation failed to complete within retry limit (attempts: 3, maxAttempts: 3) previous failures follow in order of occurrence",
636637
"Mismatch checksum value. Expected crc32c{0x00000001} actual crc32c{0xe16dcdee}",
637638
"Mismatch checksum value. Expected crc32c{0x00000002} actual crc32c{0xe16dcdee}",
638639
"Asynchronous task failed");
@@ -689,17 +690,20 @@ public void retrySettingsApplicable_objectRangeData_offset_notAligned_gt() throw
689690
try (BlobDescriptor bd = futureObjectDescriptor.get(5, TimeUnit.SECONDS)) {
690691
ApiFuture<byte[]> future = bd.readRangeAsBytes(RangeSpec.of(10L, 20L));
691692

692-
CancelledException cancelledException =
693+
OutOfRangeException outOfRangeException =
693694
assertThrows(
694-
CancelledException.class, () -> ApiExceptions.callAndTranslateApiException(future));
695+
OutOfRangeException.class,
696+
() -> ApiExceptions.callAndTranslateApiException(future));
695697

696-
assertThat(cancelledException).hasCauseThat().isInstanceOf(OutOfRangeException.class);
697-
Throwable[] suppressed = cancelledException.getSuppressed();
698+
assertThat(outOfRangeException).isInstanceOf(OutOfRangeException.class);
699+
Throwable[] suppressed = outOfRangeException.getSuppressed();
698700
List<String> suppressedMessages =
699701
Arrays.stream(suppressed).map(Throwable::getMessage).collect(Collectors.toList());
700702
assertThat(suppressedMessages)
701703
.containsExactly(
702-
"position = 10, readRange.read_offset = 11", "Asynchronous task failed");
704+
"Operation failed to complete within retry limit (attempts: 2, maxAttempts: 2) previous failures follow in order of occurrence",
705+
"position = 10, readRange.read_offset = 11",
706+
"Asynchronous task failed");
703707
}
704708
}
705709
}

0 commit comments

Comments
 (0)