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: add geometry to database args #11828

Merged
merged 34 commits into from
Oct 29, 2024

Conversation

evchip
Copy link
Contributor

@evchip evchip commented Oct 17, 2024

Closes #11819

Keeping in draft as I'd like advice on how to better handle byte size passed as CLI args, e.g. user can type "4GB" instead of "4294967296". @mattsse is there an existing convention for this? If not, I can take a stab at implementing something.

…vide custom max_size and growth_step, add branching logic in open method to use default values if args.geometry is None
…TB and 4GB, respectively; split get_database_args out with get_const_database_args to allow with_geometry as non-const; add tests for cli args max_size and growth_step
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.

ty, the CLI settings should all be optional and should support probably MB or GB parsing

crates/storage/db/src/implementation/mdbx/mod.rs Outdated Show resolved Hide resolved
crates/storage/db/src/implementation/mdbx/mod.rs Outdated Show resolved Hide resolved
…initialize geometry with default values in new() fn, change params on with_geometry to Option, remove branch logic for set_geometry() in open() fn
@guidanoli
Copy link

should support probably MB or GB parsing

And KB, if possible.

@mattsse mattsse added A-db Related to the database A-cli Related to the reth CLI labels Oct 17, 2024
@evchip
Copy link
Contributor Author

evchip commented Oct 19, 2024

should support probably MB or GB parsing

And KB, if possible.

@gerceboss any progress on this?

@gerceboss
Copy link

gerceboss commented Oct 20, 2024

I am new to reth actually, can you please guide on where exactly I will need to add this cli command for support of GB,MB or KB? @evchip

@evchip
Copy link
Contributor Author

evchip commented Oct 20, 2024

I am new to reth actually, can you please guide on where exactly I will need to add this cli command for support of GB,MB or KB? @evchip

@gerceboss I didn't hear from you so I've drafted an implementation. I'll push it shortly.

Feel free to reach out with questions in the future though.

@evchip evchip marked this pull request as ready for review October 20, 2024 09:56
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.

Last comments from my side, and then we're good. Thanks for the patience with review comments!

crates/storage/db/src/implementation/mdbx/mod.rs Outdated Show resolved Hide resolved
crates/node/core/src/args/database.rs Outdated Show resolved Hide resolved
@evchip evchip requested a review from shekhirin October 23, 2024 04:23
@evchip evchip requested a review from shekhirin October 23, 2024 08:49
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, thank you! @mattsse pending review

@shekhirin shekhirin added the C-enhancement New feature or request label Oct 23, 2024
@evchip
Copy link
Contributor Author

evchip commented Oct 24, 2024

LGTM, thank you! @mattsse pending review

Hi @mattsse, politely bumping this up for your review please.

@evchip
Copy link
Contributor Author

evchip commented Oct 28, 2024

Hi @mattsse, all tests are passing now. Please let me know if any further changes are needed. Thanks!


/// Size in bytes.
#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
pub struct ByteSize(pub usize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels weird being here tbh, but ig we can change its location later

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah this should be moved, will do separately, dont want to block this

@mattsse mattsse added this pull request to the merge queue Oct 29, 2024
Merged via the queue into paradigmxyz:main with commit 0f9ba64 Oct 29, 2024
40 checks passed
mattsse added a commit that referenced this pull request Oct 29, 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 A-db Related to the database C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Geometry to DatabaseArgs
6 participants