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

Ability to run analyzers in parallel #405

Closed
wants to merge 7 commits into from
Closed

Conversation

mitjat
Copy link
Collaborator

@mitjat mitjat commented May 4, 2023

Resolves https://app.clickup.com/t/8669qdzbh

This PR

  • adds a fast_sync key to each block analyzer's config, wherein the block range and parallelism can be specified
  • runs those fast-sync analyzers first
  • (TODO) changes "slow-sync" consensus analyzer (now at most one) so it first processes the genesis right before the first block it will analyze. If that happens to be the chain genesis document, it grabs that; otherwise, it uses StateToGenesis at an appropriate height. If it notices that at least one block after the beginning of the configured "slow" range has already been processed, it assumes genesis was already processed also, and skips genesis processing. This is different from how we've been processing genesis so far (analyze the newest chain's genesis document at most once, regardless of the configured range), but I think the new way makes more sense. It also allows the slow analyzer to do the right thing wrt genesis in any of the three scenarios:
    • it runs after fast-sync analyzers that have processed some range of blocks
    • it runs after a slow-sync analyzer (after k8s redeployment)
    • it is starting from scratch

This PR does NOT implement fast-sync behavior (= disabling dead reckoning) or parallelism tolerance inside the analyzers themselves; that happens in other PRs. So the only way to test this config in the very short term is to run "fast-sync" analyzers (that are no different from slow ones) with a parallelism level of 1.

@mitjat mitjat changed the base branch from main to andrew7234/unify-analyzer-source May 4, 2023 07:00
// Main is the main Analyzer for the consensus layer.
type Main struct {
// ConsensusAnalyzer is the main Analyzer for the consensus layer.
type ConsensusAnalyzer struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for this struct to be exported?

I think it makes more sense for NewConsensusAnalyzer to return value of type analyzer.Analyzer and then this struct can be made unexported.

I guess this will be updated in #389 anyway.

Copy link
Collaborator Author

@mitjat mitjat May 4, 2023

Choose a reason for hiding this comment

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

Urgh that's something that's been inconsistent across analyzers for a while, and I never cared enough to unify it. Fixed now; all analyzers now return the type-erased analyzer.Analyzer.

A reason (beyond not caring enough) I didn't want to touch this bit of code is that the naming was split between two conventions: Calling every analyzer Main, or calling every analyzer in module mymodule MyModuleAnalyzer. I went with the latter now; I expect opinions/preferences will differ. I don't care which one we use (beyond thinking that Main should be renamed to Analyzer or MainAnalyzer if we go there), but I don't really want to deal with bikeshedding and editing. If anybody wants to rename further, I welcome PRs (or commits to this PR).

cmd/analyzer/analyzer.go Outdated Show resolved Hide resolved
cmd/analyzer/analyzer.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
cmd/analyzer/analyzer.go Outdated Show resolved Hide resolved
@Andrew7234 Andrew7234 force-pushed the andrew7234/unify-analyzer-source branch from 8bd1aaa to 4b75ba3 Compare May 4, 2023 16:33
Base automatically changed from andrew7234/unify-analyzer-source to main May 4, 2023 16:40
@mitjat mitjat force-pushed the mitjat/parallel-analyzers branch from 13a91f8 to 465ebf7 Compare May 4, 2023 23:16
@mitjat mitjat force-pushed the mitjat/parallel-analyzers branch from 465ebf7 to f70b48e Compare May 4, 2023 23:22
@mitjat mitjat closed this Jun 16, 2023
@mitjat mitjat deleted the mitjat/parallel-analyzers branch June 16, 2023 23:51
@mitjat
Copy link
Collaborator Author

mitjat commented Jun 17, 2023

This was closed automatically when I accidentally deleted the remote (= github) branch.
Will reopen a new PR.

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.

None yet

2 participants