[services][blob] Implement Delete blob HTTP handler
ClosedPublic
Actions

Authored by bartek on Apr 17 2023, 6:09 AM.

Details

Reviewers

marcin
tomek
• jon
michal

Commits

rCOMM180715bd4bb6: [services][blob] Implement Delete blob HTTP handler

Summary

Resolves ENG-3524

THis diff implements the DELETE /blob/{holder} endpoint which revokes a blob holder. If last holder is revoked, the blob is deleted. This behavior is the same as for Remove blob RPC.

Depends on D7465

Test Plan

Created a blob with a few holders and called this endpoint to remove them one by one.
Calling GET before returns HTTP 200, after deleting returns HTTP 404 as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Apr 17 2023, 6:09 AM

bartek held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptApr 17 2023, 6:09 AM

bartek added inline comments.Apr 17 2023, 6:12 AM

services/blob/src/http/handlers/blob.rs
93–95 ↗	(On Diff #25217)	This is tracked by https://linear.app/comm/issue/ENG-414/prepare-blob-service-to-handle-cleanup

Harbormaster completed remote builds in B18409: Diff 25217.Apr 17 2023, 6:27 AM

bartek added a child revision: D7482: [services][blob] Add assign holder HTTP endpoint.Apr 18 2023, 12:00 AM

bartek published this revision for review.Apr 18 2023, 12:07 AM

• jon added inline comments.Apr 18 2023, 8:19 AM

services/blob/src/http/handlers/blob.rs
118 ↗	(On Diff #25217)	For debugging, this might be nice. Will probably need to borrow / clone `&blob_hash` earlier. If this always expected to be empty, we should probably emit the error.

tomek accepted this revision.Apr 19 2023, 2:03 AM

tomek added inline comments.

services/blob/src/http/handlers/blob.rs
96–111 ↗	(On Diff #25217)	It is theoretically possible for someone to add a new holder between these operations. This is an edge case and fixing it won't be easy.

This revision is now accepted and ready to land.Apr 19 2023, 2:03 AM

bartek added inline comments.Apr 23 2023, 10:23 PM

services/blob/src/http/handlers/blob.rs
96–111 ↗	(On Diff #25217)	Yes, unfortunately there is a possibility.
118 ↗	(On Diff #25217)	Good idea for the `debug!` here. If this always expected to be empty, we should probably emit the error. No, if it's not empty, it means there are still references to the S3 object so we delete the reference (holder) only and not the object itself