-
Notifications
You must be signed in to change notification settings - Fork 270
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
Allow configuration of replay thread pools from CLI #236
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #236 +/- ##
=========================================
- Coverage 81.9% 81.9% -0.1%
=========================================
Files 837 838 +1
Lines 226874 226913 +39
=========================================
- Hits 185870 185862 -8
- Misses 41004 41051 +47 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM. Left a couple of minor comments
3887be2
to
f117926
Compare
f117926
to
1f5bac4
Compare
Force pushed to rebase on tip of master to resolve a conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The trait will reduce redundant code and further enforce these args all being uniform with each other, both in the source code and on the CLI at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Bubble up the constants to the CLI that control the sizes of the following two thread pools: - The thread pool used to replay multiple forks in parallel - The thread pool used to execute transactions in parallel
Problem
We want to be able to control threadpool sizes on the CLI - see #105 for more context
Summary of Changes
--replay-slots-concurrently
which was a boolean with hard-coded number of threads iftrue
)--replay-slots-concurrently
argumentI did a sanity check real quick to make sure nothing broke on CLI. Namely,
--replay-slots-concurrently
and--replay-forks-threads
and observed error for conflict as expectedLastly, there is another somewhat related pool that we may wish to control, the one used to replay local ledger before
ReplayStage
is created. While that is related, I think we can push to another PR, especially since we'll want an accompanying argument added to ledger-tool as well.