feat: Unlink and cleanup ShapeStatus metadata separately
#3531
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3531 +/- ##
=======================================
Coverage 88.13% 88.13%
=======================================
Files 18 18
Lines 1643 1643
Branches 409 409
=======================================
Hits 1448 1448
Misses 193 193
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
magnetised
left a comment
magnetised
left a comment
There was a problem hiding this comment.
Do we need to remove the metadata entry immediately too?
| @spec unlink_handle_from_shape(stack_ref(), shape_handle()) :: | ||
| {:ok, Shape.t()} | {:error, term()} | ||
| def unlink_handle_from_shape(stack_ref, shape_handle) do | ||
| stack_id = extract_stack_id(stack_ref) |
There was a problem hiding this comment.
I think we also need to delete the metadata table entry here too. That's validating that a handle matches a shape for every request. If we remove the shape db entry then the metadata it would mean that no more reads come through for the shape. If we just remove the shapedb entry then a deleted handle will pass the resolve_shape_handle test without falling back to shape db.
There was a problem hiding this comment.
addressed by setting the shape hash to nil in the unlinking
As discussed this does make this whole thing less self-consistent because now has_shape_handle? will return false after unlinking, but fetch_shape_by_handle still returns the shape. I don't particularly like the API being so inconsistent so I'm contemplating whether this change is worth it given how things work now.
|
@msfstef what's the status of this PR? |
|
@alco I was sort of hoping to get #3572 in - but I think the core concept is still relevant, of deleting things async as long as the unlinking is done synchronously I've refrained from merging this as it's not strictly necessary and don't want to step over @magnetised 's work |
|
@msfstef I think the right answer here is to add a tombstone table which guards all further shape lookups and allows for a single atomic "unlink" step that guards the rest of the deletion process which, being spread across multiple tables (and soon db tables), will never be consistent. so I'm tempted to keep this work just for the new 2-phase delete. |
magnetised
force-pushed
the
msfstef/unlink-and-remove-ets-metadata-separately
branch
from
dcf0f8e to
8fc9acf
Compare
Closes #3474
Part of a larger body of work described in #3442
This is the last of the tasks that might still be relevant even with @magnetised proposed move to an SQLite backed metadata store, as we still might want to have a "quick" unlinking of a handle from the shape definition synchronously in the critical removal path, and then schedule the orphaned metadata cleanup sweep later.
I decided to still implement this for the sake of closure on the changes I proposed that I still think are relevant, the only outstanding one being #3451 which I do believe is not worth doing as lazy loading is a worse version of disk-backed DB.
I'm ok if we end up not merging this, but I think as a general API I think it might still make sense which is why I decided to at least prepare the PR.