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

ingest: Reuse Stellar-Core's cached buckets if possible. #3670

Merged
merged 14 commits into from
Jun 10, 2021

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Jun 7, 2021

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

Stellar-Core can intelligently reuse existing bucket files (stellar/stellar-core#2994) as of v17.1.0 (see the release notes). We store these under the --captive-core-storage-path=./ directory. It used to be a randomly-generated subfolder (e.g. captive-core-d34db33f/) on startup; now it's kept consistent (just captive-core/) to take advantage of this feature.

Why

Performance: faster startup. Closes #3631.

Known limitations

This feature is only available with Stellar-Core >= v17.1.0. We can introduce version checks to keep having random buckets with < 17.1, but this will complicate startup significantly, since we'd now need to run & parse stellar-core version.

Needs #3683 to be merged first for proper 17.1 support in our builds.

@Shaptic Shaptic requested a review from a team June 7, 2021 21:44
@Shaptic Shaptic self-assigned this Jun 7, 2021
@Shaptic Shaptic added this to the Horizon 2.5.0 milestone Jun 7, 2021
@ire-and-curses
Copy link
Member

The next release of Horizon will require Stellar-Core >= v17.1.0.

What's the reason Horizon is not backwards-compatible?

@Shaptic
Copy link
Contributor Author

Shaptic commented Jun 7, 2021

@ire-and-curses I think I misspoke. More accurately: to take advantage of the performance improvement, we need >= 17.1. Otherwise, I think Core will just flush the buckets folder. I'm building an older binary as we speak to make sure it's still backwards compatible. The biggest change in both cases, though, is that the directory isn't cleaned up on shutdown anymore.

@ire-and-curses
Copy link
Member

Otherwise, I think Core will just flush the buckets folder.

Ok, that's what I was expecting, thanks. Yeah as you say let's confirm for sure it will still work with an older version (and just won't be any faster).

@Shaptic Shaptic force-pushed the reuse-buckets branch 2 times, most recently from 2d472a9 to 90f29a7 Compare June 9, 2021 16:19
@bartekn
Copy link
Contributor

bartekn commented Jun 9, 2021

Otherwise, I think Core will just flush the buckets folder. I'm building an older binary as we speak to make sure it's still backwards compatible.

Have you checked this? I think Stellar-Core does not flush the folder between runs.

I checked the failing test: TestReingestDB. I think the problem is that the test starts Horizon normally in the background somewhere (maybe by our integration test framework) because the log output doesn't make sense. In reingestion we don't ingest state nor verify state but the logs suggest something else. I think if we disable starting Horizon before we actually run reingestion command it should work. It's possible that it worked before because different (random) bucket folder was used.

@Shaptic
Copy link
Contributor Author

Shaptic commented Jun 9, 2021

I think Stellar-Core does not flush the folder between runs.

I tested with 17.0 and confirmed that it does delete and redownload the bucket files @bartekn.

I think the problem is that the test starts Horizon normally in the background somewhere

Yep, I came to the same exact conclusion and introduced IntegrationTest.StopHorizon() to alleviate this in f9aaba9. All tests pass now!

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.

I tested with 17.0 and confirmed that it does delete and redownload the bucket files @bartekn.

Sounds good! On the contrary 17.1 does not delete the buckets, correct?

LGTM!

@@ -5,6 +5,9 @@ All notable changes to this project will be documented in this file. This projec

## Unreleased

### New Features
* **Performance improvement**: the Captive Core backend now reuses bucket files whenever it finds existing ones in the corresponding `--captive-core-storage-path` (introduced in [v2.0](#v2.0.0)) rather than generating a one-time temporary sub-directory ([#3670](https://github.com/stellar/go/pull/3670)). Note that taking advantage of this feature requires [Stellar-Core v17.1.0](https://github.com/stellar/stellar-core/releases/tag/v17.1.0) or later.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use value from Config instead of --captive-core-storage-path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ironically, --captive-core-storage-path means something more to Horizon than Captive Core. The corresponding BUCKET_DIR_PATH within Stellar-Core's config is a relative path to the directory from which the stellar-core command is run, so it's essentially path.Join(config.CaptiveCoreStoragePath, config.Toml.BucketDirPath) (psuedocode). Horizon uses this arg to create the directory, and that means the TOML file doesn't really apply.

We could merge these two options, but that's out of scope and requires more discussion since we'd finally be diverging from a Core-compatible TOML file.

@Shaptic
Copy link
Contributor Author

Shaptic commented Jun 10, 2021

@bartekn correct! The catchup is much faster as expected.

@Shaptic Shaptic merged commit d7c50a4 into stellar:master Jun 10, 2021
@Shaptic Shaptic deleted the reuse-buckets branch June 10, 2021 21:01
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.

Reuse Captive-Core storage path
3 participants