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: server streaming retries #1496

Merged
merged 41 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
a804c37
get branch in sync with upstream main
leahecole Apr 18, 2023
ecfb2f7
feat: update server streaming retries
leahecole Aug 22, 2023
783e736
Merge branch 'main' into gax4upgrade-2
leahecole Aug 22, 2023
25f1046
rerun npx gts fix
leahecole Aug 23, 2023
78b9e5b
fix two of three failing browser tests
leahecole Aug 23, 2023
f258a1c
Regenerate showcase (#12)
leahecole Aug 24, 2023
038ad48
fix lint
leahecole Aug 24, 2023
5c7fc4e
test: use a custom header for testing headers
alexander-fenster Aug 24, 2023
5a4bd71
rename "newRetry" parameter
leahecole Aug 30, 2023
12bd5d0
remove extend dependency
leahecole Aug 30, 2023
288f3b1
fix lint
leahecole Aug 30, 2023
8a7d5a2
fix issue where underlying errors were swallowed
leahecole Aug 31, 2023
a40172b
resolve some comments
leahecole Sep 5, 2023
ee9261f
replace console logs with our warn module
leahecole Sep 5, 2023
e5e332e
utilize enum
leahecole Sep 5, 2023
1d0d0f7
replace "any" with "GoogleError"
leahecole Sep 5, 2023
6666710
update array checks
leahecole Sep 5, 2023
a2b9cc9
remove debug statement
leahecole Sep 5, 2023
c6cccb9
remove need to typecast
leahecole Sep 5, 2023
62875cc
null coalescing fix
leahecole Sep 6, 2023
a095163
reduce duplication
leahecole Sep 6, 2023
2df47aa
clean up some optional chaining
leahecole Sep 6, 2023
152981e
make error optional
leahecole Sep 7, 2023
18b0d58
Merge branch 'main' of github.com:googleapis/gax-nodejs into gax4upgr…
leahecole Sep 25, 2023
9251c20
WIP: use nullish coalescing and optional chaining for parameter conve…
leahecole Sep 27, 2023
a737798
more nullish coalescing and optional chaining
leahecole Sep 27, 2023
1db7f49
falsiness checks
leahecole Sep 28, 2023
da3d8c3
Request/response type
leahecole Sep 29, 2023
c59005b
Request/response type
leahecole Sep 29, 2023
e09f94f
make createAPIcall nested statements more readable
leahecole Sep 29, 2023
ca8d1f7
retryCodesOrShouldRetryFn
leahecole Oct 2, 2023
deb5328
make retryCodesOrShouldRetryFn less awful
leahecole Oct 2, 2023
823e1d7
fix Sofia's comments
leahecole Oct 4, 2023
4066501
Merge branch 'main' of github.com:googleapis/gax-nodejs into gax4upgr…
leahecole Oct 5, 2023
ad6491d
Merge branch 'main' of github.com:googleapis/gax-nodejs into gax4upgr…
leahecole Oct 31, 2023
2d17563
Retrycodes refactor (#13)
leahecole Nov 10, 2023
3c92036
resolve typescript warnings
leahecole Nov 13, 2023
c41daf9
Merge branch 'main' into gax4upgrade-2
leahecole Nov 20, 2023
4953f8f
Merge branch 'main' into gax4upgrade-2
alexander-fenster Nov 27, 2023
a8488ce
fix: do not throw error if both retryCodes and shouldRetryFn are defined
alexander-fenster Nov 27, 2023
46187da
test: remove lint warnings from tests
alexander-fenster Nov 27, 2023
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/clientInterface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export interface ClientOptions
clientConfig?: gax.ClientConfig;
fallback?: boolean | 'rest' | 'proto';
apiEndpoint?: string;
gaxServerStreamingRetries?: boolean;
}

export interface Descriptors {
Expand Down
63 changes: 45 additions & 18 deletions gax/src/createApiCall.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ import {
SimpleCallbackFunction,
} from './apitypes';
import {Descriptor} from './descriptor';
import {CallOptions, CallSettings} from './gax';
import {
CallOptions,
CallSettings,
checkRetryOptions,
isRetryCodes,
} from './gax';
import {retryable} from './normalCalls/retries';
import {addTimeoutArg} from './normalCalls/timeout';
import {StreamingApiCaller} from './streamingCalls/streamingApiCaller';
Expand Down Expand Up @@ -63,7 +68,6 @@ export function createApiCall(
// function. Currently client librares are only calling this method with a
// promise, but it will change.
const funcPromise = typeof func === 'function' ? Promise.resolve(func) : func;

// the following apiCaller will be used for all calls of this function...
const apiCaller = createAPICaller(settings, descriptor);

Expand All @@ -72,9 +76,22 @@ export function createApiCall(
callOptions?: CallOptions,
callback?: APICallback
) => {
const thisSettings = settings.merge(callOptions);

let currentApiCaller = apiCaller;

let thisSettings: CallSettings;
if (currentApiCaller instanceof StreamingApiCaller) {
const gaxStreamingRetries =
currentApiCaller.descriptor?.gaxStreamingRetries;
leahecole marked this conversation as resolved.
Show resolved Hide resolved
// If Gax streaming retries are enabled, check settings passed at call time and convert parameters if needed
const thisSettingsTemp = checkRetryOptions(
leahecole marked this conversation as resolved.
Show resolved Hide resolved
callOptions,
gaxStreamingRetries
);
thisSettings = settings.merge(thisSettingsTemp);
} else {
thisSettings = settings.merge(callOptions);
}

// special case: if bundling is disabled for this one call,
// use default API caller instead
if (settings.isBundling && !thisSettings.isBundling) {
Expand All @@ -89,23 +106,33 @@ export function createApiCall(

const streaming = (currentApiCaller as StreamingApiCaller).descriptor
?.streaming;

const retry = thisSettings.retry;
if (
!streaming &&
retry &&
retry.retryCodes &&
retry.retryCodes.length > 0
) {
retry.backoffSettings.initialRpcTimeoutMillis =
retry.backoffSettings.initialRpcTimeoutMillis ||
thisSettings.timeout;
return retryable(
func,
thisSettings.retry!,
thisSettings.otherArgs as GRPCCallOtherArgs,
thisSettings.apiName
if (!streaming && retry && retry?.getResumptionRequestFn) {
throw new Error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To stay on a very safe side, can we change this to a warning, e.g. "Warning: both retryCodes and shouldRetryFn are defined for a streaming call; shouldRetryFn will take precedence", and use shouldRetryFn? Otherwise, it might start breaking existing code (very unlikely but possible) within a minor version bump.

'Resumption strategy can only be used with server streaming retries'
);
}
if (!streaming && retry && retry?.retryCodesOrShouldRetryFn) {
if (!isRetryCodes(retry.retryCodesOrShouldRetryFn) && !streaming) {
throw new Error(
'Using a function to determine retry eligibility is only supported with server streaming calls'
);
}
if (
isRetryCodes(retry.retryCodesOrShouldRetryFn) &&
retry.retryCodesOrShouldRetryFn.length > 0
) {
retry.backoffSettings.initialRpcTimeoutMillis ??=
thisSettings.timeout;
return retryable(
func,
thisSettings.retry!,
thisSettings.otherArgs as GRPCCallOtherArgs,
thisSettings.apiName
);
}
}
return addTimeoutArg(
func,
thisSettings.timeout,
Expand Down
Loading
Loading