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

Add basic failover functionality #1216

Closed
wants to merge 7 commits into from

Conversation

gtsonevv
Copy link
Collaborator

@gtsonevv gtsonevv commented Dec 8, 2023

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #733) and the maintainers have approved on my working plan.

Motivation

Added support for multiple API Keys (since we can have multiple RPC Servers)
Added basic failover functionality
With this design of failover, we will change (rotate) our server when needed and stick to this new server for our next calls. It will help to avoid long responses when first (or more) servers are down.

For now, server rotation is happening only on Timeout error.

Also, check this comment: #733 (comment)

Test Plan

Added unit tests.

Related issues/PRs

#733
#760

@gtsonevv gtsonevv requested a review from frol December 8, 2023 13:57
Copy link

changeset-bot bot commented Dec 8, 2023

🦋 Changeset detected

Latest commit: 65c6899

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@near-js/cookbook Minor
@near-js/providers Minor
@near-js/wallet-account Minor
@near-js/accounts Patch
near-api-js Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@gtsonevv gtsonevv requested a review from gagdiez December 12, 2023 09:47
Copy link
Collaborator

@vikinatora vikinatora left a comment

Choose a reason for hiding this comment

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

Looks good, I've suggested some minor changes


// Provider example
const provider = new providers.JsonRpcProvider({
// preoritized list of RPC Servers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// preoritized list of RPC Servers
// prioritized list of RPC Servers

@@ -24,36 +26,45 @@ export async function fetchJson(connectionInfoOrUrl: string | ConnectionInfo, js
} else {
connectionInfo = connectionInfoOrUrl as ConnectionInfo;
}

if (Array.isArray(connectionInfo.url) && !connectionInfo.url.length) {
throw new Error('The prioritized array of RPC Server URLs should not be empty');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
throw new Error('The prioritized array of RPC Server URLs should not be empty');
throw new Error('The prioritized array of RPC Server URLs must not be empty');

headers: { ...connectionInfo.headers, 'Content-Type': 'application/json' }
headers: {
'Content-Type': 'application/json',
'x-api-key': connectionInfo.apiKeys ? connectionInfo.apiKeys[currentRpcServer] : undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that null is semantically better than undefined in this case

Suggested change
'x-api-key': connectionInfo.apiKeys ? connectionInfo.apiKeys[currentRpcServer] : undefined,
'x-api-key': connectionInfo.apiKeys ? connectionInfo.apiKeys[currentRpcServer] : null,

@@ -333,6 +333,19 @@ export class JsonRpcProvider extends Provider {
return await this.sendJsonRpc('gas_price', [blockId]);
}

/**
* Part of the RPC failover design.
* Changing current (first) rpc server and moves it to the end of the queue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Changing current (first) rpc server and moves it to the end of the queue.
* Rotates the current RPC server with a new one (if available) and moves the former to the end of the queue.

* Changing current (first) rpc server and moves it to the end of the queue.
*/
private rotateRpcServers() {
if (typeof this.connection.url === 'string') { return; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (typeof this.connection.url === 'string') { return; }
if (typeof this.connection.url === 'string' || this.connection.url.length === 1) { return; }

try {
await provider.status();
} catch (e) {
expect(e.message).toEqual('The prioritized array of RPC Server URLs should not be empty');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
expect(e.message).toEqual('The prioritized array of RPC Server URLs should not be empty');
expect(e.message).toEqual('The prioritized array of RPC Server URLs cannot be empty');

@denbite
Copy link
Contributor

denbite commented Jan 16, 2024

The failover mechanism looks exciting to me, I've been tracking it since #760, and it's going to be very useful

However, I wanted to make sure the implementation would cover all use cases, and one of them is what if a user has different connection data for providers

For instance, how can the user provide the following list of RPCs?

[
    {
        "url": "https://rpc.mainnet.near.org",
        "timeout": 2500
    },
    {
        "url": "https://another-rpc.cloud.com",
        "timeout": 2000,
        "headers": {
            "X-Api-Key": "some-string-key"
        }
    },
    {
        "url": "https://one-more-rpc.cloud2.com",
        "headers": {
            "Authorization": "Bearer jwt.token"
        }
    }
]

And it's widespread for each provider to have its own set of headers or other parameters that are required, there's no standard for it

I propose not to extend the JsonRpcProvider class because its primary responsibility is to know how to communicate with RPC and not load-balancing providers, but instead implement the new provider class, let's say FailoverJsonRpcProvider which is going to be the only place that would know about algorithms that are used for failover

export class FailoverJsonRpcProvider extends Provider {
  constructor(
    readonly providers: JsonRpcProvider[],
    // parameters to customize the failover mechanism (retries, total time, etc)
    readonly failoverConfiguration: any
  ) {}


  ...
}

To make it work properly, we would also need to introduce the new field nodeConnections in the configuration

export interface NearConfig {
    ...
    nodeConnections: ConnectionInfo | ConnectionInfo[],
    ...
}

@gtsonevv
Please leave your thoughts about this proposal, I'd like to see it considered before merging PR

@vikinatora
Copy link
Collaborator

Hey @denbite, thank you for the feedback. On first glance your approach seems reasonable, we'll thoroughly analyze it internally and get back to you.

@frol
Copy link
Collaborator

frol commented Mar 19, 2024

It would be great to have the link to the available RPC providers also easily discoverable for the devs configuring the connection: https://docs.near.org/api/rpc/providers

@denbite
Copy link
Contributor

denbite commented Mar 21, 2024

quite a few people have been asking me recently about the failover mechanism to make their applications more robust, @vikinatora @gtsonevv do you guys have ETA for this feature?

P.S. let me know if you need any help in developing, I'd be happy to take over and deliver this great functionality to active developers as quickly as possible

@vikinatora
Copy link
Collaborator

@denbite We can give you an ETA, as we have commited to delivering other functionalities. We'd be happy for you take over and finish this functionality.

@vikinatora
Copy link
Collaborator

@gtsonevv, we can close this one.

@gagdiez
Copy link
Contributor

gagdiez commented Apr 16, 2024

closing as requested by @vikinatora

@gagdiez gagdiez closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC Redundancy/Failover Configuration
5 participants