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

cmd/satellite: create API subcommand #3280

Merged
merged 6 commits into from Oct 16, 2019

Conversation

JessicaGreben
Copy link
Contributor

@JessicaGreben JessicaGreben commented Oct 15, 2019

Why/What:

We are separating the satellite into many processes so that we can horizontally scale. Here we continue the separation satellite API and add a satellite run api subcommand to the satellite binary.

The satellite run api subcommand does the following:

  • creates a new API process
  • creates new connections to the satelliteDB, revocationDB, and pointerDB
  • creates a new API peer and checks the version is correct and initializes metrics

The subcommand does not do the following:

  • it does not run a migration for any of the databases, instead it needs to be run after the satellite run command which is responsible for the migration. We can change this in the future if need be, but for the time being, this is the simplest.

Please describe the tests:

  • Updated storj-sim to run the API process in addition to the peer process. Included with this change is that storj-sim can no longer run with sqlite since it does not handle multiple processes connecting to it very well. Instead we are requiring postgres for storj-sim. This means that anyone running storj-sim locally will need to run their own instance of postgres and supply that connection URL to the storj-sim setup command. Here is an example of how to run storj-sim now:
# first start a postrgres instance locally and create a database for storj-sim to use

$ storj-sim network setup --postgres=<connection URL>

$ storj-sim run

These steps are also outlined in the storj/docs here.

Please describe the performance impact:
This should not change performance since its all the same code. But it should help performance once deployed since we can run many replicas of the API with this setup.

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@cla-bot cla-bot bot added the cla-signed label Oct 15, 2019
@JessicaGreben JessicaGreben added Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR labels Oct 16, 2019
@JessicaGreben JessicaGreben marked this pull request as ready for review October 16, 2019 15:28
@JessicaGreben JessicaGreben requested a review from a team October 16, 2019 15:28
@ghost ghost requested review from Fadila82 and ifraixedes and removed request for a team October 16, 2019 15:28
jenlij
jenlij previously approved these changes Oct 16, 2019
cam-a
cam-a previously approved these changes Oct 16, 2019
@cam-a cam-a dismissed stale reviews from jenlij and themself via b798bfd October 16, 2019 19:23
@JessicaGreben JessicaGreben merged commit 34764e5 into master Oct 16, 2019
@JessicaGreben JessicaGreben deleted the jg/2606-split-sa-add-api-subcommand branch October 16, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Request Code Review Code review requested Reviewer Can Merge If all checks have passed, non-owner can merge PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants