feat: add new retry automated tests #1505

rishtigupta · 2025-01-15T21:39:53Z

PR Description:

Add new automated tests when there is full network outage vs when there is temporary network outage

Testing

1. Full-network outage:
Run momento-local on this branch using the following params:

cargo run -- --port 8080 --hostname 0.0.0.0 --return-error unavailable --error-rpcs get

and
Run the test using the following command:

npm run integration-test-retry-full-network-outage

2. Temporary-network outage:
Run momento-local on this branch using the following params:

cargo run -- --port 8080 --hostname 0.0.0.0 --return-error unavailable --error-rpcs get --error-count 2

and
Run the test using the following command:

npm run integration-test-retry-temporary-network-outage

Issue

closes https://github.com/momentohq/dev-eco-issue-tracker/issues/1093

packages/client-sdk-nodejs/src/internal/cache-data-client.ts

…n different test suites

pgautier404 · 2025-01-16T22:07:53Z

It looks like the test middleware is still being referenced explicitly in the cache data client. This really needs to be resolved before we can merge this PR.

malandis · 2025-01-16T22:24:11Z

packages/client-sdk-nodejs/Makefile

+test-full-network-outage-retry-service:
+	@echo "Testing full network outage retry service..."
+	@npm run integration-test-retry-full-network-outage
+
+test-temporary-network-outage-retry-service:
+	@echo "Testing temporary network outage retry service..."
+	@npm run integration-test-retry-temporary-network-outage


Do we need separate targets for these? Also the -service suffix is to distinguish services in the canaries, eg leaderboard service vs cache vs topics. You can omit that unless this is something we're going to carry over to the canaries.

malandis · 2025-01-16T22:24:55Z

packages/client-sdk-nodejs/package.json

+    "integration-test-retry-full-network-outage": "jest retry/automated-retry-full-network-outage.test.ts --maxWorkers 1 -- useConsistentReads",
+    "integration-test-retry-temporary-network-outage": "jest retry/automated-retry-temporary-network-outage.test.ts --maxWorkers 1 -- useConsistentReads",


Do we need to pull these out vs include as part of the respective services?

We can include these as part of cache service in future. As of now, the docker image for momento-local is not published limiting us to include these from running in the ci and hence the cache service.

Although I can get rid of the scripts from the package.json to eliminate the confusion.

these tests will always be run separately from the other integration tests that run against the live service though. I think separate test targets makes sense so they don't accidentally run as part of the canaries or something, but we will eventually run them in github ci

I thought we were planning on running our integration tests using momento local too in future?

Running the existing cache integration tests against momento-local will be a separate change the team can decide on later. And I think even then there will be some component of CI/CD that runs them against alpha.

But for the automated retry tests, they will only ever run against momento-local in CI, and in that case I think it would be useful to have separate Makefile and/or package.json test targets for starting them.

Example github actions workflows:

// on push to main, against alpha make test-cache-service // on pull request make test-cache-service (could be against momento-local or alpha) make test-retries-full-outage (against momento-local WITHOUT error count CLI arg) make test-retries-temporary-outage (against momento-local WITH error count CLI arg)

Okay, I am adding the targets back. We can discuss this synchronously if needed?

malandis · 2025-01-16T22:25:34Z

packages/client-sdk-nodejs/src/config/retry/fixed-count-retry-strategy.ts

@@ -35,7 +35,7 @@ export class FixedCountRetryStrategy implements RetryStrategy {
      // null means do not retry
      return null;
    }
-    if (props.attemptNumber > this.maxAttempts) {
+    if (props.attemptNumber >= this.maxAttempts) {


Was maxAttempts intended to include the initial request?

Me and Anita tried running the tests such that max retries are 3, but it always retried 4 times which was confusing.

I don't think initial request is included in retry count. I have some sample output from testing a toy program against momento-local which delayed get requests for 500ms before returning an error, and I observed ~2500ms total indicating 5 get requests, the initial one + the observed 4 (when it should've been an observed 3 retries).

logs copied here for reference

Client debug logs from above example:

Key stored successfully! Time taken for set: 3ms [2025-01-14T18:11:54.342Z] DEBUG (Momento: FixedCountRetryStrategy): Determining whether request is eligible for retry; status code: 14, request type: /cache_client.Scs/Get, attemptNumber: 0, maxAttempts: 3 [2025-01-14T18:11:54.342Z] DEBUG (Momento: FixedCountRetryStrategy): Request is eligible for retry (attempt 0 of 3, retrying immediately. [2025-01-14T18:11:54.342Z] DEBUG (Momento: RetryInterceptor): Request eligible for retry: path: /cache_client.Scs/Get; response status code: 14; number of attempts (1); will retry in 0ms [2025-01-14T18:11:54.344Z] DEBUG (Momento: RetryInterceptor): Retrying request: path: /cache_client.Scs/Get; deadline was: 2025-01-14T18:11:58.838Z, overall deadline is: 2025-01-14T18:11:58.838Z [2025-01-14T18:11:54.344Z] DEBUG (Momento: RetryInterceptor): Setting next deadline (via offset of 5000 ms) to: 2025-01-14T18:11:59.344Z [2025-01-14T18:11:54.848Z] DEBUG (Momento: FixedCountRetryStrategy): Determining whether request is eligible for retry; status code: 14, request type: /cache_client.Scs/Get, attemptNumber: 1, maxAttempts: 3 [2025-01-14T18:11:54.848Z] DEBUG (Momento: FixedCountRetryStrategy): Request is eligible for retry (attempt 1 of 3, retrying immediately. [2025-01-14T18:11:54.848Z] DEBUG (Momento: RetryInterceptor): Request eligible for retry: path: /cache_client.Scs/Get; response status code: 14; number of attempts (2); will retry in 0ms [2025-01-14T18:11:54.850Z] DEBUG (Momento: RetryInterceptor): Retrying request: path: /cache_client.Scs/Get; deadline was: 2025-01-14T18:11:59.344Z, overall deadline is: 2025-01-14T18:11:58.838Z [2025-01-14T18:11:54.850Z] DEBUG (Momento: RetryInterceptor): Setting next deadline (via offset of 5000 ms) to: 2025-01-14T18:11:59.850Z [2025-01-14T18:11:55.358Z] DEBUG (Momento: FixedCountRetryStrategy): Determining whether request is eligible for retry; status code: 14, request type: /cache_client.Scs/Get, attemptNumber: 2, maxAttempts: 3 [2025-01-14T18:11:55.358Z] DEBUG (Momento: FixedCountRetryStrategy): Request is eligible for retry (attempt 2 of 3, retrying immediately. [2025-01-14T18:11:55.358Z] DEBUG (Momento: RetryInterceptor): Request eligible for retry: path: /cache_client.Scs/Get; response status code: 14; number of attempts (3); will retry in 0ms [2025-01-14T18:11:55.359Z] DEBUG (Momento: RetryInterceptor): Retrying request: path: /cache_client.Scs/Get; deadline was: 2025-01-14T18:11:59.850Z, overall deadline is: 2025-01-14T18:11:58.838Z [2025-01-14T18:11:55.359Z] DEBUG (Momento: RetryInterceptor): Setting next deadline (via offset of 5000 ms) to: 2025-01-14T18:12:00.359Z [2025-01-14T18:11:55.868Z] DEBUG (Momento: FixedCountRetryStrategy): Determining whether request is eligible for retry; status code: 14, request type: /cache_client.Scs/Get, attemptNumber: 3, maxAttempts: 3 [2025-01-14T18:11:55.868Z] DEBUG (Momento: FixedCountRetryStrategy): Request is eligible for retry (attempt 3 of 3, retrying immediately. [2025-01-14T18:11:55.868Z] DEBUG (Momento: RetryInterceptor): Request eligible for retry: path: /cache_client.Scs/Get; response status code: 14; number of attempts (4); will retry in 0ms [2025-01-14T18:11:55.869Z] DEBUG (Momento: RetryInterceptor): Retrying request: path: /cache_client.Scs/Get; deadline was: 2025-01-14T18:12:00.359Z, overall deadline is: 2025-01-14T18:11:58.838Z [2025-01-14T18:11:55.869Z] DEBUG (Momento: RetryInterceptor): Setting next deadline (via offset of 5000 ms) to: 2025-01-14T18:12:00.869Z [2025-01-14T18:11:56.376Z] DEBUG (Momento: FixedCountRetryStrategy): Determining whether request is eligible for retry; status code: 14, request type: /cache_client.Scs/Get, attemptNumber: 4, maxAttempts: 3 [2025-01-14T18:11:56.376Z] DEBUG (Momento: FixedCountRetryStrategy): Exceeded max attempt count (3) [2025-01-14T18:11:56.376Z] DEBUG (Momento: RetryInterceptor): Request not eligible for retry: path: /cache_client.Scs/Get; retryable status code: 14; number of attempts (4). Error: The server was unable to handle the request; consider retrying. If the error persists, please contact us at [email protected]: 14 UNAVAILABLE: Unavailable Time taken for get: 2542ms Finished example!

packages/client-sdk-nodejs/test/integration/retry/automated-retry-full-network-outage.test.ts

anitarua

looking good overall! just a couple of requests

...es/client-sdk-nodejs/test/integration/retry/automated-retry-temporary-network-outage.test.ts

…use that in retry tests

packages/client-sdk-nodejs/src/internal/cache-data-client.ts

anitarua

lgtm!

would be good to get @pgautier404 and/or @malandis to sign off on the shouldLoadLate bool changes too though

pgautier404

This looks good to me, thanks! I mostly concentrated on the middleware loading portion of the code, so I'll defer to the other reviewers on the test setup and the tests themselves.

rishtigupta marked this pull request as ready for review January 15, 2025 23:49

rishtigupta force-pushed the feat/new-retry-tests branch from 7dc74f3 to 5497145 Compare January 16, 2025 00:16

pgautier404 reviewed Jan 16, 2025

View reviewed changes

packages/client-sdk-nodejs/src/internal/cache-data-client.ts Outdated Show resolved Hide resolved

anitarua reviewed Jan 16, 2025

View reviewed changes

packages/client-sdk-nodejs/src/internal/cache-data-client.ts Outdated Show resolved Hide resolved

rishtigupta mentioned this pull request Jan 16, 2025

chore: use the pull request head sha to build the ci #1506

Merged

rishtigupta added 8 commits January 16, 2025 11:58

feat: add new retry automated tests

64fa40e

feat: add make targets, refactor ful network and temp network tests i…

bf127a2

…n different test suites

fix: fix test metrics collector tests

62f4aab

fix: retry metrics collector tests

738f3d6

chore: refactor tests in retry dir

8dca6fd

fix: filter automated-retry tests from ci

1010e3d

fix: testIgnorePattern filter

ab9f2a1

fix: order of ignore test filter in the script

87375f5

rishtigupta force-pushed the feat/new-retry-tests branch from 681e0fd to 87375f5 Compare January 16, 2025 19:58

rishtigupta requested review from anitarua, pgautier404 and malandis January 16, 2025 20:19

chore: add shouldLoadLate param to the Middleware interface

f5e3758

malandis reviewed Jan 16, 2025

View reviewed changes

rishtigupta added 2 commits January 16, 2025 14:47

chore: remove targets for automated-retry tests

ea24f27

chore: add targets for automated-retry tests

406072b

anitarua reviewed Jan 17, 2025

View reviewed changes

...es/client-sdk-nodejs/test/integration/retry/automated-retry-temporary-network-outage.test.ts Outdated Show resolved Hide resolved

...es/client-sdk-nodejs/test/integration/retry/automated-retry-temporary-network-outage.test.ts Outdated Show resolved Hide resolved

chore: implement WithCacheAndCacheClient function in integ-setup and …

86db694

…use that in retry tests

rishtigupta requested review from anitarua and malandis January 17, 2025 19:32

pgautier404 reviewed Jan 17, 2025

View reviewed changes

packages/client-sdk-nodejs/src/internal/cache-data-client.ts Outdated Show resolved Hide resolved

chore: better way to initialize shouldLoadLate property in middleware

a85efa5

rishtigupta requested a review from pgautier404 January 21, 2025 18:56

anitarua approved these changes Jan 21, 2025

View reviewed changes

pgautier404 approved these changes Jan 21, 2025

View reviewed changes

malandis approved these changes Jan 21, 2025

View reviewed changes

rishtigupta merged commit 04e57f9 into main Jan 21, 2025
14 checks passed

rishtigupta deleted the feat/new-retry-tests branch January 21, 2025 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new retry automated tests #1505

feat: add new retry automated tests #1505

rishtigupta commented Jan 15, 2025 •

edited

Loading

pgautier404 commented Jan 16, 2025

malandis Jan 16, 2025

malandis Jan 16, 2025

rishtigupta Jan 16, 2025

rishtigupta Jan 16, 2025

anitarua Jan 16, 2025 •

edited

Loading

rishtigupta Jan 16, 2025

anitarua Jan 16, 2025 •

edited

Loading

rishtigupta Jan 16, 2025

malandis Jan 16, 2025

rishtigupta Jan 16, 2025

anitarua Jan 16, 2025 •

edited

Loading

anitarua left a comment

anitarua left a comment

pgautier404 left a comment

		"integration-test-retry-full-network-outage": "jest retry/automated-retry-full-network-outage.test.ts --maxWorkers 1 -- useConsistentReads",
		"integration-test-retry-temporary-network-outage": "jest retry/automated-retry-temporary-network-outage.test.ts --maxWorkers 1 -- useConsistentReads",

feat: add new retry automated tests #1505

feat: add new retry automated tests #1505

Conversation

rishtigupta commented Jan 15, 2025 • edited Loading

PR Description:

Testing

Issue

pgautier404 commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anitarua Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anitarua Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anitarua Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

anitarua left a comment

Choose a reason for hiding this comment

anitarua left a comment

Choose a reason for hiding this comment

pgautier404 left a comment

Choose a reason for hiding this comment

rishtigupta commented Jan 15, 2025 •

edited

Loading

anitarua Jan 16, 2025 •

edited

Loading

anitarua Jan 16, 2025 •

edited

Loading

anitarua Jan 16, 2025 •

edited

Loading