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.

Sign up for GitHub

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

Jump to bottom

Allow file descriptor limit to be set via CLI #24477

Merged
merged 4 commits into from
Mar 7, 2022
Merged

Allow file descriptor limit to be set via CLI #24477

merged 4 commits into from
Mar 7, 2022

Conversation

ohe
Copy link
Contributor

@ohe ohe commented Feb 27, 2022

This PR is an attempt to solve issue #24148 by allowing DatabaseHandles to be set via the Config file or the Command line interface.

By default, DatabaseHandles is set to MaxInt32. If no setting is passed (in TOML or CLI), it will be set to fdlimit.Maximum. Otherwise, the lower number between DatabaseHandles parameter & fdlimit.Maximum will be set.

rjl493456442
rjl493456442 reviewed Feb 28, 2022
View reviewed changes
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have left a few comments please check them.

cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/geth/main.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
@ohe ohe force-pushed the bugfix/allow_databasehandle_to_be_set_through_TOML branch from 32f08b5 to 349ae49 Compare February 28, 2022 10:01
@ohe
Copy link
Contributor Author

ohe commented Feb 28, 2022

I modified the content of this PR according to your remakrs @rjl493456442 .

@ohe ohe force-pushed the bugfix/allow_databasehandle_to_be_set_through_TOML branch from 349ae49 to c6320c9 Compare February 28, 2022 10:06
@ohe ohe requested a review from rjl493456442 February 28, 2022 10:48
rjl493456442
rjl493456442 approved these changes Mar 1, 2022
View reviewed changes
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

@karalabe
Copy link
Member

karalabe commented Mar 1, 2022

I'll take a closer look in a moment, I think the name will need a bit of a tweak since it only affects the db fd limit, not the global one.

@rjl493456442
Copy link
Member

I'll take a closer look in a moment, I think the name will need a bit of a tweak since it only affects the db fd limit, not the global one.

Nope, actually it refers to the global one. DB handler is still a half of it.

@karalabe
Copy link
Member

karalabe commented Mar 2, 2022

I'll take a closer look in a moment, I think the name will need a bit of a tweak since it only affects the db fd limit, not the global one.

Nope, actually it refers to the global one. DB handler is still a half of it.

Ah, good catch

@karalabe karalabe changed the title Allow DatabaseHandles to be set via CLI & TOML file Allow file descriptor limit to be set via CLI Mar 7, 2022
@karalabe
Copy link
Member

karalabe commented Mar 7, 2022

Thanks for the PR. I did take the liberty to simplify it and remove a bit of functionality.

  • There is no reason to add the max uint32 hack, it's simpler to assume that 0 (unset) means to use the current behavior. This is also less error prone as it ensures correct behavior even if some code path doesn't set the field properly for whatever reason.
  • I've added a check that if <128 file descriptors are requested, it will bump it to the system default. The 128 is a random number, it's probably even like this too low. It's mostly there to avoid shooting yourself in the foot by setting it to something low like 5.
  • I've removed the TOML support. The reasoning here is a bit complex:
    • The field being part of eth.Config feels wrong because it's a global setting for the entire Geth process, not the eth submodule.
    • The field is actually a noop within eth. If I use go-ethereum as a library instead of the Geth process, then this fdlimit field is completely ignored, which would lead to very strange bug reports.
    • Similarly to how log configs cannot be configured via TOML, only the CLI; I feel that the fdlimit is the same. It is an option for the Geth command itself, not for the go-ethereum libraries. It also needs to be applied on startup before anything Ethereum related is started (raising the OS limits).

@karalabe karalabe added this to the 1.10.17 milestone Mar 7, 2022
@karalabe karalabe merged commit a79afd9 into ethereum:master Mar 7, 2022
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Mar 8, 2022
@ohe @rjl493456442 @karalabe
cmd: allow file descriptor limit to be set via CLI (ethereum#24477)
20c8d1b 
* eth, cmd: allow FdLimit to be set in config/command line (ethereum#24148)

* eth/ethconfig: format code

* cmd, eth/ethconfig: simplify fdlimit arg, disallow toml

* cnd/utils: make fdlimit setting nicer on the logs

Co-authored-by: Gary Rong <[email protected]>
Co-authored-by: Péter Szilágyi <[email protected]>
Baro1905
Baro1905 reviewed May 3, 2022
View reviewed changes
Copy link

@Baro1905 Baro1905 left a comment

Choose a reason for hiding this comment

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

ohe:bugfix/allow_databasehandle_to_be_set_through_TOML

@ohe ohe deleted the bugfix/allow_databasehandle_to_be_set_through_TOML branch May 9, 2022 12:36
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
@ohe @rjl493456442
@karalabe @JacekGlen
cmd: allow file descriptor limit to be set via CLI (ethereum#24477)
113fa1a 
* eth, cmd: allow FdLimit to be set in config/command line (ethereum#24148)

* eth/ethconfig: format code

* cmd, eth/ethconfig: simplify fdlimit arg, disallow toml

* cnd/utils: make fdlimit setting nicer on the logs

Co-authored-by: Gary Rong <[email protected]>
Co-authored-by: Péter Szilágyi <[email protected]>
vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Jun 1, 2022
Allow file descriptor limit to be set via CLI
57e3823 
* Port of ethereum/go-ethereum#24477, which will
  be picked up in a future release of Quorum.
* We used to run with a different CLI option `--dbhandles`, I have
  retired that and used the same name as used in the upstream repo now
  that there is a CLI option for the same.
vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Jun 1, 2022
Allow file descriptor limit to be set via CLI
97f38da 
* Port of ethereum/go-ethereum#24477, which will
  be picked up in a future release of Quorum.
* We used to run with a different CLI option `--dbhandles`, I have
  retired that and used the same name as used in the upstream repo now
  that there is a CLI option for the same.
vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Jun 6, 2022
Allow file descriptor limit to be set via CLI
1e206a3 
* Port of ethereum/go-ethereum#24477, which will
  be picked up in a future release of Quorum.
* We used to run with a different CLI option `--dbhandles`, I have
  retired that and used the same name as used in the upstream repo now
  that there is a CLI option for the same.
@s1na s1na mentioned this pull request Aug 26, 2022
Merged
This was referenced Aug 26, 2022
Closed
Closed
cp-wjhan pushed a commit to cp-yoonjin/go-wemix that referenced this pull request Nov 16, 2022
@ohe @rjl493456442
@karalabe @cp-wjhan
cmd: allow file descriptor limit to be set via CLI (ethereum#24477)
3b17942 
* eth, cmd: allow FdLimit to be set in config/command line (ethereum#24148)

* eth/ethconfig: format code

* cmd, eth/ethconfig: simplify fdlimit arg, disallow toml

* cnd/utils: make fdlimit setting nicer on the logs

Co-authored-by: Gary Rong <[email protected]>
Co-authored-by: Péter Szilágyi <[email protected]>
vdamle pushed a commit to kaleido-io/quorum that referenced this pull request Feb 27, 2023
Allow file descriptor limit to be set via CLI
adbaaa7 
* Port of ethereum/go-ethereum#24477, which will
  be picked up in a future release of Quorum.
* We used to run with a different CLI option `--dbhandles`, I have
  retired that and used the same name as used in the upstream repo now
  that there is a CLI option for the same.
Brindrajsinh-Chauhan pushed a commit to kaleido-io/quorum that referenced this pull request Nov 28, 2023
@Brindrajsinh-Chauhan
Allow file descriptor limit to be set via CLI
32a12cd 
* Port of ethereum/go-ethereum#24477, which will
  be picked up in a future release of Quorum.
* We used to run with a different CLI option `--dbhandles`, I have
  retired that and used the same name as used in the upstream repo now
  that there is a CLI option for the same.
@gzliudan gzliudan mentioned this pull request Jul 10, 2024
Merged
19 tasks
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 11, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
26fcd07 
)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 17, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
2db7c06 
)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 18, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
bcd71b8 
)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 26, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
08a1cec 
)
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Jul 29, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
cb36c11 
)
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 3, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
020fac6 
)
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 19, 2024
@gzliudan
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
7041188 
)
wanwiset25 pushed a commit to XinFinOrg/XDPoSChain that referenced this pull request Aug 23, 2024
@gzliudan @wanwiset25
cmd/utils: allow file descriptor limit to be set via CLI (ethereum#24477
098f785 
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Baro1905 Baro1905 Baro1905 left review comments

@rjl493456442 rjl493456442 rjl493456442 approved these changes

@karalabe karalabe Awaiting requested review from karalabe

@holiman holiman Awaiting requested review from holiman

Assignees
No one assigned
Labels
status:triage
Projects
None yet
Milestone
1.10.17
Development

Successfully merging this pull request may close these issues.
5 participants
@ohe @karalabe @rjl493456442 @Baro1905 @ligi