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 75.17% 75.17%
=======================================
Files 51 51
Lines 2743 2743
Branches 408 404 -4
=======================================
Hits 2062 2062
Misses 679 679
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.
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.