Skip to content

Conversation

@anushka567
Copy link
Member

@anushka567 anushka567 commented Nov 5, 2025

Description

As part of non atomic rename, after creating the object at the destination, we delete the object at the source. If it is already cached, we invalidate it. However, the invalidation should happen only after the deletion at source is successful.

Mimicking the same as done in Unlink() method.

Link to the issue in case of a bug fix.

#3974

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - Automated

Any backward incompatible change? If so, please explain.

@anushka567 anushka567 requested a review from a team as a code owner November 5, 2025 07:13
@anushka567 anushka567 added the execute-integration-tests Run only integration tests label Nov 5, 2025
@anushka567 anushka567 added the execute-integration-tests-on-zb To run E2E tests on zonal bucket. label Nov 5, 2025
@github-actions github-actions bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Nov 5, 2025
@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.10%. Comparing base (6e04005) to head (2231e13).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
internal/fs/fs.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3977       +/-   ##
===========================================
+ Coverage        0   83.10%   +83.10%     
===========================================
  Files           0      148      +148     
  Lines           0    18146    +18146     
===========================================
+ Hits            0    15081    +15081     
- Misses          0     2516     +2516     
- Partials        0      549      +549     
Flag Coverage Δ
unittests 83.10% <75.00%> (?)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@anushka567 anushka567 enabled auto-merge (squash) November 5, 2025 09:56
oldParent.Lock()
defer oldParent.Unlock()

err = oldParent.DeleteChildFile(
Copy link
Collaborator

@ashmeenkaur ashmeenkaur Nov 5, 2025

Choose a reason for hiding this comment

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

deleteChildFile can throw an error in case of stale object Generation or Metageneration (I think it will return ESTALE error in that case).
IMO it is best to invalidate the entry in cache for those cases so follow up requests can succeed by not using the stale cached generation/metageneration.

Can you also check if the above hypothesis is correct?

@anushka567 anushka567 disabled auto-merge November 5, 2025 10:25
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

Hi @kislaykishore, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@anushka567 anushka567 removed the request for review from kislaykishore November 6, 2025 10:39
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 execute-integration-tests-on-zb To run E2E tests on zonal bucket. remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants