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

MINOR: [C++][Azure][FS] Document some limitations and atomicity guarantees #40838

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

felipecrv
Copy link
Contributor

Rationale for this change

Documenting some details of the behavior of destructive filesystem operations.

What changes are included in this PR?

Only docstring changes.

Are these changes tested?

N/A.

@felipecrv felipecrv requested a review from kou March 27, 2024 14:13
@felipecrv
Copy link
Contributor Author

@Tom-Newton

Copy link
Contributor

@Tom-Newton Tom-Newton left a comment

Choose a reason for hiding this comment

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

One comment but it looks go to me

Comment on lines +287 to +288
/// accounts. A check is made to guarantee the parent directory doesn't
/// disappear after the blob is deleted and while this operation is running,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this referring to flat namespace accounts only? I can't imagine a hierarchical namespace directory disappearing when a file is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-wording suggestion? My intention is being concise, but yeah, none of this applies to HNS accounts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Less concise but I would go for

Suggested change
/// accounts. A check is made to guarantee the parent directory doesn't
/// disappear after the blob is deleted and while this operation is running,
/// accounts. On flat namespace storage accounts a check is made to guarantee the parent directory doesn't
/// disappear after the blob is deleted and while this operation is running,

I don't feel strongly

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 27, 2024
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Mar 28, 2024
@felipecrv felipecrv merged commit edf7e57 into apache:main Mar 28, 2024
40 checks passed
@felipecrv felipecrv removed the awaiting merge Awaiting merge label Mar 28, 2024
@felipecrv felipecrv deleted the azure_docs branch March 28, 2024 15:21
Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit edf7e57.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

tolleybot pushed a commit to tmct/arrow that referenced this pull request May 2, 2024
…ntees (apache#40838)

### Rationale for this change

Documenting some details of the behavior of destructive filesystem operations.

### What changes are included in this PR?

Only docstring changes.

### Are these changes tested?

N/A.

Authored-by: Felipe Oliveira Carvalho <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
…ntees (apache#40838)

### Rationale for this change

Documenting some details of the behavior of destructive filesystem operations.

### What changes are included in this PR?

Only docstring changes.

### Are these changes tested?

N/A.

Authored-by: Felipe Oliveira Carvalho <[email protected]>
Signed-off-by: Felipe Oliveira Carvalho <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants