feat(sync-service): Validate request handles using shape hash #3515
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3515 +/- ##
==========================================
- Coverage 75.31% 75.28% -0.04%
==========================================
Files 51 51
Lines 2743 2743
Branches 405 409 +4
==========================================
- Hits 2066 2065 -1
- Misses 675 676 +1
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
force-pushed
the
magnetised/validate-shape-params-using-hash
branch
from
7120731 to
b793517
Compare
|
Beat me to it, this is exactly what I was also thinking while working on shape status! gmta (can't review but saw this pop up and was happy haha) |
msfstef
left a comment
msfstef
left a comment
There was a problem hiding this comment.
I think this is a net positive to maintain the benefits of requests where a shape handle is provided!
For the next step of what to do with the shape def -> handle lookup, I think I would go a step further than what you discuss in the followup issues, happy to chat about it!
| def dependency_handles_known?(%__MODULE__{} = shape), | ||
| do: shape.shape_dependencies_handles != [] | ||
|
|
||
| def hash(%__MODULE__{} = shape), |
There was a problem hiding this comment.
I'm against having two separate places in code that return the "hash" of the shape, as they risk going out of sync with each other. I think we can deprecate this hash call and use comparable_hash everywhere we need the hash potentially. IMO a single public API returns the hash.
magnetised
force-pushed
the
magnetised/validate-shape-params-using-hash
branch
2 times, most recently
from
89cf5c1 to
e7d419c
Compare
…idation This is part 1 of #3511. Rather than loading a shape handle from the ShapeStatus LUTs using the Shape definition and then checking that it matches the one in the request params, we instead do a simple check that the request's handle and shape definition match using a hash equality check. We only do a shape def -> handle lookup if the request doesn't have a handle parameter or the hash check fails Fixes #3512
#3518) This is essentially what we have now but with a clear api between fast metadata lookups and shape -> handle and handle -> shape lookups. This moves the %Shape{} instances out of the metadata table and gives a hint of the memory savings to come, this is for ~150K (very simple) shapes: ``` shapedb:shape_lookup:single_stack | 149432 | 242.7575 MiB shapedb:handle_lookup:single_stack | 149432 | 83.1469 MiB shape_meta_table:single_stack | 149432 | 29.5633 MiB shape_relation_lookup_table:single_s | 149432 | 19.3010 MiB shape_last_used_table:single_stack | 149432 | 15.8808 MiB ``` You can see that the shapedb tables are responsible for most of the ets usage and the metadata now only consumes ~30MiB, so a ~90% reduction, which means we shouldn't have to go any further and move more things out of ram. The current api is not going to work for a sqlite backed version, but it's close I think. Fixes #3513
magnetised
force-pushed
the
magnetised/validate-shape-params-using-hash
branch
from
e7d419c to
2c4f4c0
Compare
This is part 1 of #3511.
Rather than loading a shape handle from the ShapeStatus LUTs using the Shape definition and then checking that it matches the one in the request params, we instead do a simple check that the request's handle and shape definition match using a hash equality check.
We only do a shape def -> handle lookup if the request doesn't have a handle parameter or the hash check fails.
I've also replaced use of
fetch_shape_by_handleas the implementation ofhas_shape?with a lookup that just uses metadata.This is an intermediate step - the next PR will move the shape out of the meta table completely and encapsulate shape lookups as a separate module.
Fixes #3512