Skip to content

polygon: Apply RoTx limit - #11963

Merged
taratorio merged 3 commits into
mainfrom
shohamc1/rwtxlimit
Sep 12, 2024
Merged

taratorio merged 3 commits into
mainfrom
shohamc1/rwtxlimit

Conversation

@shohamc1

Copy link
Copy Markdown
Contributor

Fixes #11899

@shohamc1 shohamc1 changed the title polygon: Apply RwTx limit polygon: Apply RoTx limit Sep 12, 2024
@taratorio
taratorio enabled auto-merge (squash) September 12, 2024 10:23
}

heimdallReader, err = heimdall.AssembleReader(ctx, cc.Bor.CalculateSprintNumber, cfg.DataDir, cfg.Dirs.Tmp, logger)
heimdallReader, err = heimdall.AssembleReader(ctx, cc.Bor.CalculateSprintNumber, cfg.DataDir, cfg.Dirs.Tmp, logger, roTxLimit)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be possible to combine cfg.DataDir, cfg.Dirs.Tmp, logger and roTxLimit to a DBConfig struct to avoid adding yet another parameter?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, I agree with Elton, I actually raised the same thing in a previous PR - #11924 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I even suggest we have separate config structs per component like

  • heimdall.ReaderConfig and bridge.ReaderConfig
  • heimdall.ServiceConfig and bridge.ServiceConfig
  • etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to keep this productive though, maybe this PR can be merged and these improvements can be done in a follow up?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, I'm approving and leaving it up to Shoham.

@taratorio
taratorio merged commit c5d3284 into main Sep 12, 2024
@taratorio
taratorio deleted the shohamc1/rwtxlimit branch September 12, 2024 10:52
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.

Make Astrid heimdall and bridge DB respect db.read.concurrency flag

3 participants