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

[syncer/storage] Pre-store Blocks #269

Merged
merged 30 commits into from
Dec 8, 2020
Merged

Conversation

patrick-ogrady
Copy link
Contributor

@patrick-ogrady patrick-ogrady commented Dec 7, 2020

To increase sync speed, we modified the storage/modules package to pre-store block data before it is sequenced (which can be done concurrently). This significantly reduces the amount of work that must be done serially (i.e. updating balances, updating coins, etc).

WARNING: THIS PR INCLUDES BREAKING CHANGES TO STORAGE! YOU MUST RESYNC ANY DATA STORED USING PREVIOUS RELEASES!

Changes

  • Pre-store blocks as soon as they are seen by the syncer (instead of waiting to store until blocks are sequenced)
  • Replace accountEntry with *types.AccountCurrency
  • Manually encode/decode *types.AccountCurrency
  • Ensure ctx changes in syncer do not introduce regression in rosetta-cli
  • Update syncer tests to handle BlockSeen
  • limit concurrent seen calls to runtime.NumCPU (tend to be more intensive than fetch calls so allow for separate configuration)
    • add ability to override concurrency in config (SeenConcurrency)

Future Work

  • Add configuration option to rosetta-cli to determine SeeBlock concurrency

@coveralls
Copy link

coveralls commented Dec 7, 2020

Pull Request Test Coverage Report for Build 13474

  • 208 of 289 (71.97%) changed or added relevant lines in 4 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 78.078%

Changes Missing Coverage Covered Lines Changed/Added Lines %
storage/modules/balance_storage.go 23 25 92.0%
syncer/syncer.go 55 60 91.67%
storage/modules/block_storage.go 28 49 57.14%
storage/encoder/encoder.go 102 155 65.81%
Files with Coverage Reduction New Missed Lines %
syncer/syncer.go 1 91.18%
storage/modules/block_storage.go 3 76.37%
Totals Coverage Status
Change from base Build 13199: -0.3%
Covered Lines: 8074
Relevant Lines: 10341

💛 - Coveralls

syncer/types.go Outdated Show resolved Hide resolved
syncer/types.go Outdated Show resolved Hide resolved
@patrick-ogrady patrick-ogrady changed the title [syncer/storage] Pre-store Blocks [WIP][syncer/storage] Pre-store Blocks Dec 7, 2020
@patrick-ogrady patrick-ogrady changed the title [WIP][syncer/storage] Pre-store Blocks [syncer/storage] Pre-store Blocks Dec 7, 2020
@@ -38,8 +38,7 @@ lint-examples:
golangci-lint run -v -E ${LINT_SETTINGS}

lint: | lint-examples
golangci-lint run --timeout 2m0s -v -E ${LINT_SETTINGS},gomnd && \
make check-comments;
golangci-lint run --timeout 2m0s -v -E ${LINT_SETTINGS},gomnd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the double context.Context in sequenceBlocks causes a linting issue that can't be bypassed 😢

syncer/syncer.go Outdated Show resolved Hide resolved

func (s *Syncer) sequenceBlocks( // nolint:golint
ctx context.Context,
pipelineCtx context.Context,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to spawn up more fetcher threads.

syncer/syncer.go Outdated Show resolved Hide resolved
yfl92
yfl92 previously approved these changes Dec 8, 2020
Copy link

@yfl92 yfl92 left a comment

Choose a reason for hiding this comment

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

LGTM

@patrick-ogrady patrick-ogrady force-pushed the patrick/encounter-block branch from d571721 to 9aca901 Compare December 8, 2020 16:14
@patrick-ogrady patrick-ogrady requested a review from yfl92 December 8, 2020 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants