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.

Sign up for GitHub

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

Jump to bottom

Add multiple web3 provider support to discovery's eth web3 provider #1298

Merged
merged 10 commits into from
Mar 26, 2021
Merged

Add multiple web3 provider support to discovery's eth web3 provider #1298

merged 10 commits into from
Mar 26, 2021

Conversation

cheran-senthil
Copy link
Contributor

@cheran-senthil cheran-senthil commented Mar 12, 2021 •
edited

Description

This PR adds a new config variable eth_provider_url to the discovery provider to allow for multiple comma separated eth web3 providers that can be selected with equal probability.

This allows for greater resiliency since now the services will fall back to a different provider in case of failure.

This has not been added to data web3 because of disparate block status reads.

Tests

Tested against local deployment,

  • Tested both environment variables with one valid and and one invalid web3 provider; both valid; both invalid.
  • Tested signup, track upload, track upload, and user profile update.
    • This does not work for a single invalid provider as expected.
    • Works when one of the provided URLs is valid.

Other

This code needs closer review since it changes the tests to the discovery service.

@cheran-senthil cheran-senthil added discovery-node Discovery Node (previously known as Discovery Provider) requires-special-attention This change is risky and/or critical and requires special attention in review labels Mar 12, 2021
@cheran-senthil cheran-senthil marked this pull request as ready for review March 13, 2021 02:33
raymondjacobson
raymondjacobson previously approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@raymondjacobson raymondjacobson left a comment

Choose a reason for hiding this comment

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

seems reasonable!

discovery-provider/src/utils/multi_provider.py Show resolved Hide resolved
discovery-provider/src/utils/multi_provider.py Show resolved Hide resolved
@cheran-senthil cheran-senthil force-pushed the vss-discovery-web3-multi-provider branch from 3698ca7 to 5c0a362 Compare March 17, 2021 03:16
SidSethi
SidSethi reviewed Mar 18, 2021
View reviewed changes
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

looks pretty good! and thanks for the clear testing description

couple small things

discovery-provider/default_config.ini Outdated Show resolved Hide resolved
discovery-provider/src/utils/helpers.py Outdated Show resolved Hide resolved
discovery-provider/src/utils/multi_provider.py Show resolved Hide resolved
@cheran-senthil
Address PR comments:
3b21d29 
* Add docstring
* Add comment
SidSethi
SidSethi previously approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

great work!

discovery-provider/default_config.ini Outdated Show resolved Hide resolved
hareeshnagaraj
hareeshnagaraj suggested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

How are we going to handle disparate block status reads from different web3 providers?

@cheran-senthil
Copy link
Contributor Author

Updated to use multiple providers only for eth.

@cheran-senthil cheran-senthil force-pushed the vss-discovery-web3-multi-provider branch from 069d505 to 09cdbd0 Compare March 24, 2021 04:15
hareeshnagaraj
hareeshnagaraj previously approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making the changes 🙏

SidSethi
SidSethi previously approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

looks great!
please update PR description/title to reflect the actual code changes - don't see any mention that ethWeb3 only now uses MultiProvider

discovery-provider/tests/conftest.py Outdated Show resolved Hide resolved
discovery-provider/src/__init__.py Show resolved Hide resolved
@cheran-senthil cheran-senthil changed the title Add multiple web3 provider support to discovery nodes Add multiple web3 provider support to discovery's eth web3 provider Mar 26, 2021
@cheran-senthil
Copy link
Contributor Author

Thanks, I've updated the PR description and the comments

SidSethi
SidSethi previously requested changes Mar 26, 2021
View reviewed changes
discovery-provider/tests/conftest.py Outdated Show resolved Hide resolved
@SidSethi
Copy link
Contributor

also since mad-dog is flaky, lets re-run it a couple times to see if it works or post logs indicating successful local mad-dog run . annoying that it is not reliable atm :/

hareeshnagaraj
hareeshnagaraj approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

Let's get em

@cheran-senthil cheran-senthil merged commit feeb3a6 into master Mar 26, 2021
@cheran-senthil cheran-senthil deleted the vss-discovery-web3-multi-provider branch March 26, 2021 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hareeshnagaraj hareeshnagaraj hareeshnagaraj approved these changes

@raymondjacobson raymondjacobson Awaiting requested review from raymondjacobson

@SidSethi SidSethi Awaiting requested review from SidSethi

Assignees
No one assigned
Labels
discovery-node Discovery Node (previously known as Discovery Provider) requires-special-attention This change is risky and/or critical and requires special attention in review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cheran-senthil @SidSethi @raymondjacobson @hareeshnagaraj @dmanjunath