feat: segregate retry decision for read and write APIs

momentohq · Nov 12, 2024 · 29c0ee5 · 29c0ee5
1 parent 757d61d
commit 29c0ee5
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 21 deletions.
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
@@ -5,7 +5,7 @@ import {
   EligibleForRetryProps,
 } from './eligibility-strategy';
 
-const retryableGrpcStatusCodes: Array<Status> = [
+const retryableWriteGrpcStatusCodes: Array<Status> = [
   // including all the status codes for reference, but
   // commenting out the ones we don't want to retry on for now.
 
@@ -28,35 +28,63 @@ const retryableGrpcStatusCodes: Array<Status> = [
   // Status.UNAUTHENTICATED
 ];
 
-const retryableRequestTypes: Array<string> = [
+const retryableReadGrpcStatusCodes: Array<Status> = [
+  // 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,
+  // 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 retryableWriteRequestTypes: Array<string> = [
   '/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.
    *  In the future, we may also add "the first/last N occurrences of a value".
    *  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<string> = [
+  '/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;
 
@@ -65,20 +93,23 @@ export class DefaultEligibilityStrategy implements EligibilityStrategy {
   }
 
   isEligibleForRetry(props: EligibleForRetryProps): boolean {
-    if (!retryableGrpcStatusCodes.includes(props.grpcStatus.code)) {
-      this.logger.debug(
-        `Response with status code ${props.grpcStatus.code} is not retryable.`
-      );
-      return false;
+    if (
+      retryableReadGrpcStatusCodes.includes(props.grpcStatus.code) &&
+      retryableReadRequestTypes.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;
+    if (
+      retryableWriteGrpcStatusCodes.includes(props.grpcStatus.code) &&
+      retryableWriteRequestTypes.includes(props.grpcRequest.path)
+    ) {
+      return true;
     }
 
-    return true;
+    this.logger.debug(
+      `Request with type ${props.grpcRequest.path} and status code ${props.grpcStatus.code} is not retryable.`
+    );
+    return false;
   }
 }
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
@@ -58,7 +58,23 @@ describe('DefaultEligibilityStrategy', () => {
     expect(isEligible).toBe(true);
   });
 
-  it('should return false for CANCELLED status code and GET request path', () => {
+  it('should return true for CANCELLED status code and GET request path', () => {
+    const grpcStatus = {code: Status.CANCELLED} as StatusObject;
+    const grpcRequest = {
+      path: '/cache_client.Scs/Get',
+    } as ClientMethodDefinition<unknown, unknown>;
+    const requestMetadata = new Metadata();
+
+    const isEligible = eligibilityStrategy.isEligibleForRetry({
+      grpcStatus,
+      grpcRequest,
+      requestMetadata,
+    });
+
+    expect(isEligible).toBe(true);
+  });
+
+  it('should return false for CANCELLED status code and SET request path', () => {
     const grpcStatus = {code: Status.CANCELLED} as StatusObject;
     const grpcRequest = {
       path: '/cache_client.Scs/Set',