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

fix: Counter does increment first retry #1601

Merged
merged 24 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c726d2d
Commented out for testing
danieljbruce May 14, 2024
4a4fc53
Add a bunch of logs
danieljbruce May 16, 2024
8e75244
more logs
danieljbruce May 16, 2024
590fd13
Save the logs on a branch
danieljbruce May 16, 2024
0ab3ad3
Add mechanism for setting retries to 0
danieljbruce May 16, 2024
9299121
Logging for unique bug
danieljbruce May 16, 2024
aa9fe2e
Add a testShouldFailOnThirdError test
danieljbruce May 17, 2024
26fb2c4
move retries counter back
danieljbruce May 17, 2024
2b964c2
Update code to increment counter on first retry
danieljbruce May 21, 2024
f1fc02f
Remove two tests that are not used
danieljbruce May 21, 2024
4151a59
Remove the comment code
danieljbruce May 21, 2024
de14552
Add some error codes
danieljbruce May 21, 2024
b87679b
Remove console logs scattered throughout code
danieljbruce May 21, 2024
1615aa2
Remove logging variables
danieljbruce May 21, 2024
dd19e2c
Remove a couple tests that are run twice
danieljbruce May 21, 2024
3eb195d
Undo settings changes
danieljbruce May 21, 2024
bc84f76
Debugging and finding retries parameter
danieljbruce May 21, 2024
2eafc35
Remove comments that are not needed
danieljbruce May 21, 2024
994ae29
Simplify diff
danieljbruce May 21, 2024
7a62dce
Remove comment that is not true anymore
danieljbruce May 21, 2024
cb5e750
Remove from handler. It is not needed.
danieljbruce May 21, 2024
48b84f4
Update gax/test/test-application/src/index.ts
danieljbruce May 21, 2024
b454fd6
Remove a console log
danieljbruce May 21, 2024
0777f15
Merge branch 'counter-does-increment-first-retry' of https://github.c…
danieljbruce May 21, 2024
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
1 change: 1 addition & 0 deletions gax/src/streamingCalls/streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ export class StreamProxy extends duplexify implements GRPCCallResult {
this.destroy();
return; //end chunk
} else {
this.retries!++;
retryStream = this.retry(stream, retry);
this.stream = retryStream;
return retryStream;
Expand Down
73 changes: 72 additions & 1 deletion gax/test/test-application/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ async function testShowcase() {
await testCollect(grpcClientWithServerStreamingRetries);
await testChat(grpcClientWithServerStreamingRetries);
await testWait(grpcClientWithServerStreamingRetries);
await testShouldFailOnThirdError(
grpcSequenceClientWithServerStreamingRetries
);
}

function createStreamingSequenceRequestFactory(
Expand Down Expand Up @@ -657,7 +660,7 @@ async function testServerStreamingRetrieswithRetryRequestOptions(
const finalData: string[] = [];
const retryRequestOptions = {
objectMode: true,
retries: 1,
retries: 2,
maxRetryDelay: 70,
danieljbruce marked this conversation as resolved.
Show resolved Hide resolved
retryDelayMultiplier: 3,
totalTimeout: 650,
Expand Down Expand Up @@ -711,6 +714,74 @@ async function testServerStreamingRetrieswithRetryRequestOptions(
});
}

// When maxRetries are set to 2 then on the third error from the server gax
// should throw an error that says the retry count has been exceeded.
async function testShouldFailOnThirdError(client: SequenceServiceClient) {
const backoffSettings = createBackoffSettings(
100,
1.2,
1000,
null,
1.5,
3000,
null
);
const allowedCodes = [4, 5, 6];
const retryOptions = new RetryOptions(allowedCodes, backoffSettings);
backoffSettings.maxRetries = 2;

const settings = {
retry: retryOptions,
};

client.initialize();

const request = createStreamingSequenceRequestFactory(
[
Status.DEADLINE_EXCEEDED, // Error code 4
Status.NOT_FOUND, // Error code 5
Status.ALREADY_EXISTS, // Error code 6
Status.OK,
],
[0.1, 0.1, 0.1, 0.1],
[0, 0, 0, 1],
'This is testing the brand new and shiny StreamingSequence server 3'
);
const response = await client.createStreamingSequence(request);
await new Promise<void>((resolve, reject) => {
const sequence = response[0];

const attemptRequest =
new protos.google.showcase.v1beta1.AttemptStreamingSequenceRequest();
attemptRequest.name = sequence.name!;

const attemptStream = client.attemptStreamingSequence(
attemptRequest,
settings
);
attemptStream.on('data', () => {
reject(new GoogleError('The stream should not receive any data'));
});
attemptStream.on('error', (error: GoogleError) => {
try {
assert.strictEqual(error.code, 4);
assert.strictEqual(
error.message,
'Exceeded maximum number of retries before any response was received'
);
resolve();
} catch (assertionError: unknown) {
reject(assertionError);
}
});
attemptStream.on('end', () => {
reject(
new GoogleError('The stream should not end before it receives an error')
);
});
});
}

// streaming call that retries twice with RetryRequestOpsions and resumes from where it left off
async function testServerStreamingRetrieswithRetryRequestOptionsResumptionStrategy(
client: SequenceServiceClient
Expand Down
27 changes: 14 additions & 13 deletions gax/test/unit/streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1348,19 +1348,20 @@ describe('handles server streaming retries in gax when gaxStreamingRetries is en
);

call.on('error', err => {
assert(err instanceof GoogleError);
if (err.code !== 14) {
// ignore the error we are expecting
assert.strictEqual(err.code, 4);
// even though max retries is 2
// the retry function will always be called maxRetries+1
// the final call is where the failure happens
assert.strictEqual(retrySpy.callCount, 3);
assert.strictEqual(
err.message,
'Exceeded maximum number of retries before any response was received'
);
done();
try {
assert(err instanceof GoogleError);
if (err.code !== 14) {
// ignore the error we are expecting
assert.strictEqual(err.code, 4);
assert.strictEqual(retrySpy.callCount, 2);
assert.strictEqual(
err.message,
'Exceeded maximum number of retries before any response was received'
);
done();
}
} catch (error: unknown) {
done(error);
leahecole marked this conversation as resolved.
Show resolved Hide resolved
}
});
});
Expand Down
Loading