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/expingest: Ingest into memory only flag #2299

Merged

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Feb 20, 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

This commit adds a config flag that makes ingestion system to ingest into memory structures only.

Close #2250.

Why

Currently it's not possible to move ingestion system entirely to backend server. Because of this a spike in requests to frontend servers may increase CPU usage and slow down ingestion. To solve this, it's now possible to set INGEST_IN_MEMORY_ONLY config flag that would make the instance wait until other ingesting instance updates a DB and then it will just catchup ingesting into memory structures only.

Known limitations

This requires more tests before releasing so probably shouldn't be included in 1.0.0.

@cla-bot cla-bot bot added the cla: yes label Feb 20, 2020
@bartekn bartekn requested review from tamirms and abuiles February 20, 2020 15:16
@bartekn
Copy link
Contributor Author

bartekn commented Feb 20, 2020

I realized that we should also check the condition in historyRangeState state (and possibly just in start). Basically, when INGEST_IN_MEMORY_ONLY is set, the instance should be able to operate with read-only DB connection. EDIT: fixed in 8d0d358.

@ire-and-curses
Copy link
Member

Nice! With this change, do I understand correctly that it would be possible to have front-end instances that serve e.g. the orderbook graph from memory, without having them be ingesters writing to the DB? That would imply that we could then split a cluster into true front end (serving requests) and backend (pure ingestion to DB), correct?

@bartekn
Copy link
Contributor Author

bartekn commented Feb 20, 2020

@ire-and-curses exactly!

@tamirms tamirms linked an issue Feb 20, 2020 that may be closed by this pull request
@tamirms
Copy link
Contributor

tamirms commented Feb 20, 2020

@bartekn I like the idea you mentioned of using a read only connection. Along the same lines, I think you can set the ingestion transaction to be read only by calling BeginTx(&sql.TxOptions{ReadOnly: true}). Would it make sense to have a function called beginIngestionTx() which creates a transaction that is read only iff config.IngestInMemoryOnly is true? Then all the state machine nodes can call beginIngestionTx() to create their transactions

@tamirms
Copy link
Contributor

tamirms commented Feb 21, 2020

@bartekn it turns out that postgres does not allow select for update queries on read only transactions. if we want to restrict ingestion to use read only connections when config.IngestInMemoryOnly is true, we can either use a postgres advisory lock to replace the select for update or, we can avoid locking entirely when config.IngestInMemoryOnly is true.

I've confirmed that postgres advisory locks do work on read only transactions. The advantage of this approach is that it maintains the existing locking behavior.

I think that it is probably safe for us to avoid locking when config.IngestInMemoryOnly is true but I'm not 100% sure because it's hard to analyze all the possible scenarios. The advantage of avoiding locking is that, if we have many read only horizon nodes it is possible for those read only nodes to delay ingestion if they repeatedly acquire the lock before the horizon ingestion nodes. But, I'm not sure how much of an issue this would be in practice.

@bartekn
Copy link
Contributor Author

bartekn commented Feb 21, 2020

@tamirms thanks for checking this! I prefixed the flag with EXP_ in 951c730 so let's merge it without using read-only transaction. I think you are right about not locking in IngestInMemoryOnly mode - let's confirm it while working on the next version.

@bartekn bartekn merged commit 4ca71cc into stellar:release-horizon-v0.25.0 Feb 21, 2020
@bartekn bartekn deleted the ingest-into-memory-only branch February 21, 2020 12:21
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.

Ingestion nodes cannot be separated from request serving horizon nodes
3 participants