handle failed api inits #1970

guplersaxanoid · 2023-08-24T04:09:57Z

Description

Avoid exiting process when one of the API initialization fails. Instead mark it is as unusable and continue initializing other APIs.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

Checklist

I have tested locally
I have performed a self review of my changes
Updated any relevant documentation
Linked to any relevant issues
I have added tests relevant to my changes
Any dependent changes have been merged and published in downstream modules
My code is up to date with the base branch
I have updated relevant changelogs. We suggest using chan

stwiname

If the initial connection fails will it ever be retried again?

stwiname · 2023-08-24T04:41:37Z

packages/node-core/src/indexer/connectionPoolState.manager.ts

    const poolItem: ConnectionPoolItem<T> = {
      primary: primary,
      performanceScore: 100,
      failureCount: 0,
      endpoint: endpoint,
      backoffDelay: 0,
      rateLimited: false,
-      failed: false,
+      failed: initFailed,


Do we need to add initFailed can't we use this?

stwiname · 2023-08-24T04:42:31Z

packages/node-core/src/indexer/connectionPoolState.manager.ts

+      .filter((index) => !this.pool[index].initFailed);
+
+    if (initedIndices.length === 0) {
+      throw new Error(`Initialization failed for all endpoints. Please add healthier endpoints.`);


This doesn't seem like the right place to throw this error. This isn't an init function

stwiname · 2023-08-24T04:42:55Z

packages/node/src/indexer/api.service.ts

+          if (
+            network.chainId &&
+            network.chainId !== this.networkMeta.genesisHash
+          ) {
+            const err = new Error(
+              `Network chainId doesn't match expected genesisHash. Your SubQuery project is expecting to index data from "${
+                network.chainId ?? network.genesisHash
+              }", however the endpoint that you are connecting to is different("${
+                this.networkMeta.genesisHash
+              }). Please check that the RPC endpoint is actually for your desired network or update the genesisHash.`,
+            );
+            logger.error(err, err.message);
+            throw err;
+          }
+        } else {
+          const genesisHash = api.genesisHash.toString();
+          if (this.networkMeta.genesisHash !== genesisHash) {
+            throw this.metadataMismatchError(
+              'Genesis Hash',
+              this.networkMeta.genesisHash,
+              genesisHash,
+            );
+          }
        }
-      }

-      endpointToApiIndex[endpoint] = connection;
+        endpointToApiIndex[endpoint] = connection;
+      } catch (e) {
+        logger.error(`failed to init ${endpoint}: ${e}`);
+        endpointToApiIndex[endpoint] = null;


Can we move some of this to node core? Its pretty much the same on all chains

guplersaxanoid · 2023-08-24T06:30:01Z

If the initial connection fails will it ever be retried again?

I'm not sure how to handle this when there are multiple workers involved. the initialization may fail in a subset of workers and we should only allow using an endpoint when all of the workers are connected to it.

stwiname · 2023-08-24T21:20:40Z

If the initial connection fails will it ever be retried again?

I'm not sure how to handle this when there are multiple workers involved. the initialization may fail in a subset of workers and we should only allow using an endpoint when all of the workers are connected to it.

Why would we only use an endpoint if all workers are connected? The shared state is the weighting not what endpoints can be used

stwiname

This is looking like a good improvement

stwiname · 2023-08-27T21:45:58Z

packages/node-core/src/api.service.ts

+  ): void {
+    if (attempt < 5) {
+      //eslint-disable-next-line @typescript-eslint/no-misused-promises
+      setTimeout(async () => {


It might be worth having a backoff function as we use the same concept a few places now. Also there needs to be a reference to this timeout

stwiname · 2023-08-27T21:46:57Z

packages/node-core/src/indexer/connectionPool.service.ts

+      throw new Error(`Attempting to update connection that does not exist.`);
+    }
+
+    this.allApi[index] = api;


Do you think this is safe? What happens if the wrong index is provided?

1.) the order of the api will be different for different workers, making the connection pool state invalid because only the index is used to reference api.
2.) one api might overwrite another

so, it's important that we get the index right.

Yes its important to get the index right. Is using an int the right thing to key these by? If we use the endpoint it would be much safer

stwiname · 2023-08-29T01:39:41Z

packages/node/src/indexer/api.service.ts

@@ -47,8 +48,9 @@ export class ApiService
    connectionPoolService: ConnectionPoolService<ApiPromiseConnection>,
    eventEmitter: EventEmitter2,
    private nodeConfig: NodeConfig,
+    retryManager: RetryManager,


I was just meaning a util function. Adding another dependency ads a lot of unnecessary complexity.

Its already hard enough to write tests with ApiService because we need all the connectionPool stuff. We should be able to mock an api service without all that when we want a single endpoint. But thats another issue that shouldn't be addressed here

stwiname

Awesome! I think changing from index to endpoint was a good idea, it seems to mak code more readable

stwiname · 2023-08-30T21:53:38Z

I tried this by adding wss://fake.example.com/public-ws to the polkadot starter and the project wont start. The OnFinality endpoint is there still

stwiname · 2023-09-05T23:06:25Z

packages/node-core/src/api.service.ts

+    });
+
+    try {
+      const {fulfilledIndex, result} = (await raceFulfilled(connectionPromises)) as {


Why is there a cast needed here?

to rule out undefined

handle failed api inits

cf42702

guplersaxanoid requested a review from stwiname August 24, 2023 04:09

guplersaxanoid added 3 commits August 24, 2023 04:16

update changelog

9a4ff1d

fix worker connection pool state interface

81e7f65

fix IConnectionPoolStateManager

ec36e19

stwiname reviewed Aug 24, 2023

View reviewed changes

move connection creation and validation logic to node-core

ce76d2c

ianhe8x mentioned this pull request Aug 25, 2023

Improve Multiple Endpoints Robustness #1945

Open

attempt retries for failed connections

97a6102

stwiname reviewed Aug 27, 2023

View reviewed changes

guplersaxanoid added 2 commits August 28, 2023 07:36

implement retry manager

34f5778

rebase

47e8f39

stwiname requested changes Aug 29, 2023

View reviewed changes

guplersaxanoid added 4 commits August 29, 2023 03:21

add util function for retry with backoff

b96ae09

reference api by endpoints in connection pool

2a2ee52

rebase

42cab93

fix tests

dde4087

stwiname approved these changes Aug 30, 2023

View reviewed changes

promise resolution with raceFullfiled

808cc5a

stwiname reviewed Sep 5, 2023

View reviewed changes

Merge branch 'main' into api-init-fix

c1ba3be

stwiname approved these changes Sep 6, 2023

View reviewed changes

stwiname merged commit 40a24d4 into main Sep 6, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle failed api inits #1970

handle failed api inits #1970

guplersaxanoid commented Aug 24, 2023

stwiname left a comment

stwiname Aug 24, 2023

stwiname Aug 24, 2023

stwiname Aug 24, 2023

guplersaxanoid commented Aug 24, 2023

stwiname commented Aug 24, 2023

stwiname left a comment

stwiname Aug 27, 2023

stwiname Aug 27, 2023

guplersaxanoid Aug 28, 2023

stwiname Aug 29, 2023

stwiname Aug 29, 2023

stwiname left a comment

stwiname commented Aug 30, 2023

stwiname Sep 5, 2023

guplersaxanoid Sep 6, 2023

handle failed api inits #1970

handle failed api inits #1970

Conversation

guplersaxanoid commented Aug 24, 2023

Description

Type of change

Checklist

stwiname left a comment

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

guplersaxanoid commented Aug 24, 2023

stwiname commented Aug 24, 2023

stwiname left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stwiname left a comment

Choose a reason for hiding this comment

stwiname commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment