Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

chore(storage-monitor): improve free_space calculation and cli default value#14133

Merged
paritytech-processbot[bot] merged 6 commits into
paritytech:masterfrom
yjhmelody:fix-storage-monitor
May 12, 2023
Merged

chore(storage-monitor): improve free_space calculation and cli default value#14133
paritytech-processbot[bot] merged 6 commits into
paritytech:masterfrom
yjhmelody:fix-storage-monitor

Conversation

@yjhmelody
Copy link
Copy Markdown
Contributor

@yjhmelody yjhmelody commented May 12, 2023

No description provided.

yjhmelody
yjhmelody commented May 12, 2023
View reviewed changes
Comment thread client/storage-monitor/src/lib.rs
davxy
davxy reviewed May 12, 2023
View reviewed changes
Comment thread client/storage-monitor/src/lib.rs
/// Returns free space in MB, or error if statvfs failed.
fn free_space(path: &Path) -> Result<u64, Error> {
Ok(fs4::available_space(path).map(|s| s / 1_000_000)?)
Ok(fs4::available_space(path).map(|s| s / 1024 / 1024)?)
Copy link
Copy Markdown
Member

@davxy davxy May 12, 2023

Choose a reason for hiding this comment

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

I think that the returned value was purposely returned in MB and not in MiB.
But maybe @michalkucharczyk knows better

Copy link
Copy Markdown
Contributor

@michalkucharczyk michalkucharczyk May 12, 2023

Choose a reason for hiding this comment

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

Well I made this decision basing on https://en.wikipedia.org/wiki/Megabyte being 10^6 not 1024^2.
Feel free to change it if you really feel like we need to use MiB (Mebibyte). But please also adjust clap args documentation.

Copy link
Copy Markdown
Member

@davxy davxy May 12, 2023

Choose a reason for hiding this comment

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

If we return the value in MiB then also change the docs (MB -> MiB) please

Copy link
Copy Markdown
Contributor

@michalkucharczyk michalkucharczyk May 12, 2023

Choose a reason for hiding this comment

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

also: #13466 (comment)

Copy link
Copy Markdown
Contributor Author

@yjhmelody yjhmelody May 12, 2023

Choose a reason for hiding this comment

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

Well, I think we should use MiB(1024 * 1024 Byte).
This is more accurate for node consumption of storage resources.

Copy link
Copy Markdown
Contributor

@michalkucharczyk michalkucharczyk May 12, 2023

Choose a reason for hiding this comment

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

ok, for me. but please update docs/comments wherever needed.

@davxy davxy requested a review from michalkucharczyk May 12, 2023 15:16
@yjhmelody
Copy link
Copy Markdown
Contributor Author

yjhmelody commented May 12, 2023

I changed all MB to MiB.

michalkucharczyk
michalkucharczyk suggested changes May 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

Please update docs/comments: MB -> MiB

@yjhmelody yjhmelody changed the title chore(storage-monitor): fix free_space calculation chore(storage-monitor): improve free_space calculation and cli default value May 12, 2023
@yjhmelody
Copy link
Copy Markdown
Contributor Author

yjhmelody commented May 12, 2023 •
edited
Loading

Please update docs/comments: MB -> MiB

Where? I do not know the docs. Comments have been updated all.

michalkucharczyk
michalkucharczyk reviewed May 12, 2023
View reviewed changes
Comment thread client/storage-monitor/src/lib.rs Outdated
@yjhmelody @michalkucharczyk
Update client/storage-monitor/src/lib.rs
21f7293 
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
@michalkucharczyk michalkucharczyk added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 12, 2023
ggwpez
ggwpez approved these changes May 12, 2023
View reviewed changes
Comment thread client/storage-monitor/src/lib.rs Outdated
Comment on lines +49 to +52
#[arg(long = "db-storage-polling-period", value_name = "SECONDS", default_value_t = 5, value_parser = clap::value_parser!(u32).range(1..))]
#[arg(long = "db-storage-polling-period", value_name = "SECONDS", default_value_t = 6, value_parser = clap::value_parser!(u32).range(1..))]
Copy link
Copy Markdown
Member

@ggwpez ggwpez May 12, 2023

Choose a reason for hiding this comment

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

Why was this bumped?

Comment thread client/storage-monitor/src/lib.rs Outdated
Comment on lines +45 to +48
#[arg(long = "db-storage-threshold", value_name = "MB", default_value_t = 1000)]
#[arg(long = "db-storage-threshold", value_name = "MiB", default_value_t = 1000)]
Copy link
Copy Markdown
Member

@ggwpez ggwpez May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When you start using base 2, maybe also use 1024 here so the default is one GiB and not some odd mixture.

bkchr
bkchr approved these changes May 12, 2023
View reviewed changes
Comment thread client/storage-monitor/src/lib.rs Outdated
Comment thread client/storage-monitor/src/lib.rs Outdated
@bkchr
Copy link
Copy Markdown
Member

bkchr commented May 12, 2023

bot merge

@paritytech-processbot
Copy link
Copy Markdown

paritytech-processbot Bot commented May 12, 2023

Waiting for commit status.

@paritytech-processbot paritytech-processbot Bot merged commit d3bb3e8 into paritytech:master May 12, 2023
@yjhmelody yjhmelody deleted the fix-storage-monitor branch May 13, 2023 03:52
@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@yjhmelody @michalkucharczyk
@bkchr @nathanwhit
chore(storage-monitor): improve free_space calculation and cli defa…
c8bc6a7 
…ult value (paritytech#14133)

* chore(storage-monitor): fix free_space calculation

* add `Result` type

* add docs

* update `polling_period` default value to `6`

* Update client/storage-monitor/src/lib.rs

Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>

* Apply suggestions from code review

---------

Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@bkchr bkchr bkchr approved these changes

@davxy davxy davxy approved these changes

@ggwpez ggwpez ggwpez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yjhmelody @bkchr @michalkucharczyk @davxy @ggwpez