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

feat: --db.exclusive flag for nfs volumes #7346

Merged
merged 5 commits into from Mar 28, 2024
Merged

Conversation

argakiig
Copy link
Contributor

The purpose of this PR is to further extend #6755 and allow for
--db.exclusive=true to be used to support nfs volumes

the purpose of this is to further extend paradigmxyz#6755 and allow for `--db.exclusive=true` to be used to support nfs volumes
@argakiig argakiig requested a review from gakonst as a code owner March 26, 2024 22:11
@onbjerg onbjerg added C-enhancement New feature or request A-cli Related to the reth CLI labels Mar 27, 2024
@onbjerg onbjerg changed the title Update database_args.rs to enable nfs support with flag feat: --db.exclusive flag for nfs volumes Mar 27, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Please also update the command help in the book: https://github.com/paradigmxyz/reth/tree/main/book/cli/reth

* update book help text
* linting
@@ -12,13 +12,17 @@ pub struct DatabaseArgs {
/// Database logging level. Levels higher than "notice" require a debug build.
#[arg(long = "db.log-level", value_enum)]
pub log_level: Option<LogLevel>,
/// Open environment in exclusive/monopolistic mode. Enabling allows nfs volumes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Open environment in exclusive/monopolistic mode. Enabling allows nfs volumes
/// Open environment in exclusive/monopolistic mode. Makes it possible to open a database on an NFS volume.

Comment on lines 437 to 441
--db.exclusive <EXCLUSIVE>
Exclusion mode enabled or not

[possible values: true, false]

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be generated with make update-book-cli, the description of the argument should match the rustdoc comment.

* fix verbiage for rustdoc
* run update-book-cli
*note this caught more than just my changes*
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

pending @shekhirin

@@ -12,13 +12,18 @@ pub struct DatabaseArgs {
/// Database logging level. Levels higher than "notice" require a debug build.
#[arg(long = "db.log-level", value_enum)]
pub log_level: Option<LogLevel>,
/// Open environment in exclusive/monopolistic mode. Makes it possible to open a database on an
/// NFS volume.
#[arg(long = "db.exclusive", exclusive = false)]
Copy link
Collaborator

@shekhirin shekhirin Mar 28, 2024

Choose a reason for hiding this comment

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

do we need exclusive here? https://docs.rs/clap/latest/clap/struct.Arg.html#method.exclusive

it doesn't set the default value for the argument, think we should rather leave it as None when not specified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah OK that makes sense. Thanks for the link to the docs as well. Change incoming

Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

.

remove exclusive flag based on misunderstanding of call, default works like exclusive=false anyways so comes across as redundant
@argakiig argakiig requested a review from shekhirin March 28, 2024 16:59
Copy link
Collaborator

@shekhirin shekhirin 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!

@shekhirin shekhirin added this pull request to the merge queue Mar 28, 2024
Merged via the queue into paradigmxyz:main with commit d022b5b Mar 28, 2024
27 checks passed
Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024
Ruteri pushed a commit to Ruteri/reth that referenced this pull request Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants