-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
…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
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.
ty, the CLI settings should all be optional and should support probably MB or GB parsing
…initialize geometry with default values in new() fn, change params on with_geometry to Option, remove branch logic for set_geometry() in open() fn
…ault trait impl for DatabaseArgs
And KB, if possible. |
@gerceboss any progress on this? |
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. |
… for parsing max_size and growth_step from strings
…ax_size and growth_step params
…nit into two assertions
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.
Last comments from my side, and then we're good. Thanks for the patience with review comments!
…nd add comments to describe args; remove ByteSize import
…ze> in with_geometry
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, thank you! @mattsse pending review
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); |
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.
feels weird being here tbh, but ig we can change its location later
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.
yeah this should be moved, will do separately, dont want to block this
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.