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

Delete existing empty dest folder before rename #2568

Merged
merged 12 commits into from
Oct 14, 2024

Conversation

Tulsishah
Copy link
Collaborator

@Tulsishah Tulsishah commented Oct 9, 2024

Description

  • Renaming a directory into an existing empty directory in HNS was causing an error from GCS.
  • To support this feature and maintain consistency with FLAT buckets, we manually delete the empty directory before performing the rename operation.

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - Added
  3. Integration tests - Added and run with automation https://fusion2.corp.google.com/ci/kokoro/prod%3Agcsfuse%2Fgcp_ubuntu%2Fpresubmits%2Fperf_tests%2Fpresubmit/activity/da24a85e-e032-4361-a8d2-103ee3b7bc29
    Without the fix, the additional test was failing and I could see RenameFolder error from the logs.

@Tulsishah Tulsishah changed the title Add DeleteFolder command to rename folder with existing empty folder Add DeleteFolder to rename folder with existing empty folder Oct 9, 2024
@Tulsishah Tulsishah added the execute-integration-tests Run only integration tests label Oct 9, 2024
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.28%. Comparing base (f2d240d) to head (4aa449a).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2568      +/-   ##
==========================================
+ Coverage   78.20%   78.28%   +0.08%     
==========================================
  Files         107      107              
  Lines       11802    11805       +3     
==========================================
+ Hits         9230     9242      +12     
+ Misses       2081     2071      -10     
- Partials      491      492       +1     
Flag Coverage Δ
unittests 78.28% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Tulsishah Tulsishah changed the title Add DeleteFolder to rename folder with existing empty folder Delete existing empty dest folder before rename Oct 10, 2024
@Tulsishah Tulsishah marked this pull request as ready for review October 10, 2024 11:28
@Tulsishah Tulsishah requested a review from a team as a code owner October 10, 2024 11:28
@kislaykishore kislaykishore requested review from a team, tritone and gargnitingoogle and removed request for a team and tritone October 10, 2024 11:29
@kislaykishore kislaykishore removed the request for review from ankitaluthra1 October 10, 2024 11:30
Copy link
Collaborator

@ankitaluthra1 ankitaluthra1 left a comment

Choose a reason for hiding this comment

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

minor comments, also could you please add integration test links in description Test section, the new test and an old test where rename fails if directory is not empty and rename passes when dest directory doesnt exist,

internal/fs/fs.go Outdated Show resolved Hide resolved
internal/fs/fs.go Outdated Show resolved Hide resolved
@Tulsishah
Copy link
Collaborator Author

minor comments, also could you please add integration test links in description Test section, the new test and an old test where rename fails if directory is not empty and rename passes when dest directory doesnt exist,

I've added it, and you can open it through the Kokoro button from the checks section. All the old tests are passing, and I've added a new test case where the destination directory exists and is empty. You can see the successful results from Kokoro in the passes results section.

@ankitaluthra1 ankitaluthra1 merged commit 0718955 into master Oct 14, 2024
13 checks passed
@ankitaluthra1 ankitaluthra1 deleted the fix_rename_folder_existing_empty_dest_dir branch October 14, 2024 04:45
internal/fs/hns_bucket_test.go Show resolved Hide resolved
internal/fs/hns_bucket_test.go Show resolved Hide resolved
internal/fs/hns_bucket_test.go Show resolved Hide resolved
internal/fs/fs.go Show resolved Hide resolved
internal/fs/fs.go Show resolved Hide resolved
@gargnitingoogle
Copy link
Collaborator

Oh I just noticed that this got merged in the middle of my review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execute-integration-tests Run only integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants