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

services/horizon/internal: Create separate db connections for ingestion #2560

Merged
merged 3 commits into from
May 8, 2020

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented May 7, 2020

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Fixes #2018

Create separate db connections to the horizon and stellar core databases for ingestion.

Why

Currently, all of the Horizon services share the same DB connection pool. In case of increased number of HTTP requests, it's possible that ingestion service will have to wait for an idle connection.

Known limitations

[N/A]

@bartekn
Copy link
Contributor

bartekn commented May 7, 2020

We can probably merge it into master as previous PRs for this release (no breaking changes).

@tamirms tamirms changed the base branch from release-horizon-v1.3.0 to master May 8, 2020 03:07
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

LGTM! Added two comments but I don't have a strong opinion on any.

@@ -19,33 +19,70 @@ import (
"github.com/stellar/go/support/log"
)

func mustInitHorizonDB(app *App) {
session, err := db.Open("postgres", app.config.DatabaseURL)
const maxIngestionDBConnections = 2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it belongs to the ingest package. Say, we add a new routine that require an extra DB connection. Would be easier to increment it there.

// and another pool for ingestion. Ideally the total connections in both
// pools should be bounded by HorizonDBMaxOpenConnections. But, if the ingestion
// pool consumes a significant quota of HorizonDBMaxOpenConnections then we will
// allow a total of HorizonDBMaxOpenConnections + maxIngestionDBConnections connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just return an error (maybe at a command validation stage in root.go) when ingestion connections is equal/greater than max set by a user. The reason is that user may explicitly set the max number of open connections in Postgres config so these extra 2 connections won't be established (and will probably generate a bunch of errors in the log).

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.

services/horizon/expingest: Separate DB connection pool for ingestion service
3 participants