diff --git a/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts b/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts index 176a2d2ec..8cc3eea8b 100644 --- a/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts +++ b/packages/client-sdk-nodejs/src/config/retry/default-eligibility-strategy.ts @@ -5,36 +5,11 @@ import { EligibleForRetryProps, } from './eligibility-strategy'; -const retryableWriteGrpcStatusCodes: Array = [ +const retryableGrpcStatusCodes: Array = [ // including all the status codes for reference, but // commenting out the ones we don't want to retry on for now. // Status.OK, - // Status.CANCELLED, - // Status.UNKNOWN, - // Status.INVALID_ARGUMENT, - // Status.DEADLINE_EXCEEDED, - // Status.NOT_FOUND, - // Status.ALREADY_EXISTS, - // Status.PERMISSION_DENIED, - // Status.RESOURCE_EXHAUSTED, - // Status.FAILED_PRECONDITION, - // Status.ABORTED, - // Status.OUT_OF_RANGE, - // Status.UNIMPLEMENTED, - Status.INTERNAL, - Status.UNAVAILABLE, - // Status.DATA_LOSS, - // Status.UNAUTHENTICATED -]; - -const retryableReadGrpcStatusCodes: Array = [ - // including all the status codes for reference, but - // commenting out the ones we don't want to retry on for now. - - // Status.OK, - // Read requests can be safely retried for CANCELLED errors. These may pop us sometimes during - // client or server side deployments Status.CANCELLED, // Status.UNKNOWN, // Status.INVALID_ARGUMENT, @@ -53,17 +28,23 @@ const retryableReadGrpcStatusCodes: Array = [ // Status.UNAUTHENTICATED ]; -const retryableWriteRequestTypes: Array = [ +const retryableRequestTypes: Array = [ '/cache_client.Scs/Set', + '/cache_client.Scs/Get', '/cache_client.Scs/Delete', '/cache_client.Scs/DictionarySet', // not idempotent: '/cache_client.Scs/DictionaryIncrement', + '/cache_client.Scs/DictionaryGet', + '/cache_client.Scs/DictionaryFetch', '/cache_client.Scs/DictionaryDelete', '/cache_client.Scs/SetUnion', + '/cache_client.Scs/SetDifference', + '/cache_client.Scs/SetFetch', // not idempotent: '/cache_client.Scs/ListPushFront', // not idempotent: '/cache_client.Scs/ListPushBack', // not idempotent: '/cache_client.Scs/ListPopFront', // not idempotent: '/cache_client.Scs/ListPopBack', + '/cache_client.Scs/ListFetch', /* * Warning: in the future, this may not be idempotent * Currently it supports removing all occurrences of a value. @@ -71,20 +52,11 @@ const retryableWriteRequestTypes: Array = [ * In the latter case it is not idempotent. */ '/cache_client.Scs/ListRemove', + '/cache_client.Scs/ListLength', // not idempotent: '/cache_client.Scs/ListConcatenateFront', // not idempotent: '/cache_client.Scs/ListConcatenateBack' ]; -const retryableReadRequestTypes: Array = [ - '/cache_client.Scs/Get', - '/cache_client.Scs/DictionaryGet', - '/cache_client.Scs/DictionaryFetch', - '/cache_client.Scs/SetDifference', - '/cache_client.Scs/SetFetch', - '/cache_client.Scs/ListFetch', - '/cache_client.Scs/ListLength', -]; - export class DefaultEligibilityStrategy implements EligibilityStrategy { private readonly logger: MomentoLogger; @@ -93,23 +65,20 @@ export class DefaultEligibilityStrategy implements EligibilityStrategy { } isEligibleForRetry(props: EligibleForRetryProps): boolean { - if ( - retryableReadGrpcStatusCodes.includes(props.grpcStatus.code) && - retryableReadRequestTypes.includes(props.grpcRequest.path) - ) { - return true; + if (!retryableGrpcStatusCodes.includes(props.grpcStatus.code)) { + this.logger.debug( + `Response with status code ${props.grpcStatus.code} is not retryable.` + ); + return false; } - if ( - retryableWriteGrpcStatusCodes.includes(props.grpcStatus.code) && - retryableWriteRequestTypes.includes(props.grpcRequest.path) - ) { - return true; + if (!retryableRequestTypes.includes(props.grpcRequest.path)) { + this.logger.debug( + `Request with type ${props.grpcRequest.path} is not retryable.` + ); + return false; } - this.logger.debug( - `Request with type ${props.grpcRequest.path} and status code ${props.grpcStatus.code} is not retryable.` - ); - return false; + return true; } } diff --git a/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts b/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts index 4808e9648..0a862689c 100644 --- a/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts +++ b/packages/client-sdk-nodejs/test/unit/config/retry/default-eligibility-strategy.test.ts @@ -74,7 +74,7 @@ describe('DefaultEligibilityStrategy', () => { expect(isEligible).toBe(true); }); - it('should return false for CANCELLED status code and SET request path', () => { + it('should return true for CANCELLED status code and SET request path', () => { const grpcStatus = {code: Status.CANCELLED} as StatusObject; const grpcRequest = { path: '/cache_client.Scs/Set', @@ -87,6 +87,22 @@ describe('DefaultEligibilityStrategy', () => { requestMetadata, }); + expect(isEligible).toBe(true); + }); + + it('should return false for CANCELLED status code and dictionary increment request path', () => { + const grpcStatus = {code: Status.CANCELLED} as StatusObject; + const grpcRequest = { + path: '/cache_client.Scs/DictionaryIncrement', + } as ClientMethodDefinition; + const requestMetadata = new Metadata(); + + const isEligible = eligibilityStrategy.isEligibleForRetry({ + grpcStatus, + grpcRequest, + requestMetadata, + }); + expect(isEligible).toBe(false); }); });