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

Server returns Blob_Deleted for deleted blob out of retention #2821

Merged

Conversation

justinlin-linkedin
Copy link
Collaborator

Summary

When issuing a get request with include_deleted get option, frontend would send a get request to server to fetch the content of the blob. However, if any chunk blob is deleted and out of retention window, even if we have metadata chunk still in the server, we can't finish downloading the entire blob.

This PR fixes this issue by not returning content for metadata. Now if client issues a get request with include_deleted get option and trying to download a blob that was already deleted and out of retention window, server would just return ID_Deleted error back to frontend so frontend would just return 410 back to client right away.

To download deleted and out of retention blobs, we can issue a get request with include_all.

Details

Add a new StoreGetOptions Store_Include_Compaction_Ready

Change the meaning of Store_Include_Deleted

Test

Unit test

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.21%. Comparing base (52ba813) to head (6ef975a).
Report is 41 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2821      +/-   ##
============================================
+ Coverage     64.24%   70.21%   +5.96%     
- Complexity    10398    11756    +1358     
============================================
  Files           840      842       +2     
  Lines         71755    72362     +607     
  Branches       8611     8709      +98     
============================================
+ Hits          46099    50806    +4707     
+ Misses        23004    18896    -4108     
- Partials       2652     2660       +8     

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

* This option indicates that the store needs to return the message as long as the message
* has not been physically deleted from the store.
*/
Store_Include_Compaction_Ready
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: currently we have several user who enable Store_Include_Deleted to get the blob as long as it not been physically deleted. So seems like this is not a backward compatible change. Will this affect any of their current use cases?

Store_Include_Deleted
Store_Include_Deleted,
/**
* This option indicates that the store needs to return the message as long as the message
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ. Do we need this new option? Would it be easier if we tell customers that Store_Include_Deleted only works if they issue GET within the delete retention window?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason to add this option is to make sure that Include_All get option would continue to work for compaction_ready blobs.

@justinlin-linkedin justinlin-linkedin merged commit 1d989b0 into linkedin:master Jul 16, 2024
5 checks passed
@justinlin-linkedin justinlin-linkedin deleted the justin/server-delete branch July 16, 2024 23:49
justinlin-linkedin added a commit to justinlin-linkedin/ambry that referenced this pull request Jul 19, 2024
snalli pushed a commit that referenced this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants