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

filesystemAvailable and related functions support one optional argument with disk name #42064

Merged
merged 3 commits into from Nov 22, 2022
Merged

filesystemAvailable and related functions support one optional argument with disk name #42064

merged 3 commits into from Nov 22, 2022

Conversation

ucasfl
Copy link
Collaborator

@ucasfl ucasfl commented Oct 4, 2022 •
edited

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

filesystemAvailable and related functions support one optional argument with disk name, and change filesystemFree to filesystemUnreserved. Closes #35076.

@robot-ch-test-poll robot-ch-test-poll added the pr-improvement Pull request with some product improvements label Oct 4, 2022
@CurtizJ CurtizJ self-assigned this Oct 5, 2022
CurtizJ
CurtizJ reviewed Oct 15, 2022
View reviewed changes
src/Functions/filesystem.cpp
{
static constexpr auto name = "filesystemFree";
static std::uintmax_t get(const std::filesystem::space_info & spaceinfo) { return spaceinfo.free; }
static constexpr auto name = "filesystemUnreserved";
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we can change the name now, because this function was added a long time ago. Maybe add an alias for old name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But filesystemFree is not same as filesystemUnreserved, and we can not get free space from IDisk interface, so I changed the function to filesystemUnreserved.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Then, let's mention it in changelog and mark PR as backward-incompatible.

@ucasfl ucasfl requested a review from CurtizJ November 15, 2022 15:00
@CurtizJ
Copy link
Member

CurtizJ commented Nov 16, 2022

@ucasfl

Some changed tests are flaky. Could you please fix them?

@ucasfl
Copy link
Collaborator Author

ucasfl commented Nov 16, 2022

@ucasfl

Some changed tests are flaky. Could you please fix them?

I have no idea why the tests become flaky?

@CurtizJ CurtizJ mentioned this pull request Nov 22, 2022
Merged
CurtizJ added a commit that referenced this pull request Nov 22, 2022
@CurtizJ
Merge pull request #43461 from CurtizJ/merging-42064
2572202 
Merging #42064
@CurtizJ CurtizJ merged commit 5532e3d into ClickHouse:master Nov 22, 2022
@CurtizJ
Copy link
Member

CurtizJ commented Nov 22, 2022

@ucasfl

I have no idea why the tests become flaky?

The reason was that filesystemAvailable and filesystemUnreserved, which is implemented via filesystemAvailable are called non-atomically and condition filesystemAvailable() >= filesystemUnreserved can fail if amount of disk space changes between calls.

I fixed it in #43461 and merged it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CurtizJ CurtizJ Awaiting requested review from CurtizJ

Assignees

@CurtizJ CurtizJ

Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

filesystemAvailable and similar functions should take optional argument with disk name.
3 participants
@ucasfl @CurtizJ @robot-ch-test-poll