stash: split out a stash-types crate #22240
Conversation
|
This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having a knowledgeable reviewer, it may be useful to add observability and/or a feature flag. What's This?
Buggy File Hotspots:
|
src/stash/src/upgrade/v28_to_v29.rs
Outdated
| } | ||
| }; | ||
| objects_v29::RoleId { value } | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
@jkosh44 It makes me a bit nervous that I messed something up for these to be unused. Is that expected?
There was a problem hiding this comment.
I'm not sure, it's possible I left these in by mistake and they weren't needed. Why are you changing all of these From impls into normal functions?
There was a problem hiding this comment.
Because the structs they're defined on now live in mz_stash_types, so rust doesn't let us impl traits on them here (mz_stash) anymore
There was a problem hiding this comment.
Ah ok, I looked through and I don't see this used anywhere. I probably just added it out of instinct after adding the database id function above.
There was a problem hiding this comment.
Think the other ones are the same story? (There are a few)
There was a problem hiding this comment.
Yes, sorry the other ones too. I looked through the file and I use most of those types in the tests, so I probably mistakenly thought I needed these functions, but I don't.
|
those before/after dependency graphs... 😂 |
src/stash/src/upgrade/v28_to_v29.rs
Outdated
| } | ||
| }; | ||
| objects_v29::RoleId { value } | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Ah ok, I looked through and I don't see this used anywhere. I probably just added it out of instinct after adding the database id function above.
|
TFTR! |
|
Oh lolol cargo check on just mz-stash is even more dramatic (because the protos all got moved out) Before After |
This allows for faster iteration on mz-stash itself, as is happening in Pv2. Conceptually, stash is only used in envd (by the catalog and by the storage controller) but it was being pulled in by all sorts of crates for the objects protos. Separate out the latter (plus StashError) into a new mz-stash-types crate that is still depended on by lots of things, but changes less often. The various old versions of objects.proto want to be next to it, so pull them into mz-stash-types, too. In an ideal world, we'd probably move the vX_to_vX+1 upgrades also, but they depend on actual stash things (e.g. Transaction), so leave them where they are. Finally, any given metric can only be registered once (otherwise we get a panic), but we need two stashes, so also pull out the stash Metrics into mz-stash-types. Ideally, this would have stayed where it was. A possible alternative to this would be something like putting a type-safe cache of registered metrics objects on MetricsRegistry. Timing on my M1 laptop of bin/environmentd after invalidating stash went from 2m12s to 53s. A large part of this is likely that we've entirely eliminated clusterd, including linking. ``` touch src/stash/src/lib.rs && time ./bin/environmentd -- --nope ```
3 participants
This allows for faster iteration on mz-stash itself, as is happening in Pv2.
Conceptually, stash is only used in envd (by the catalog and by the storage controller) but it was being pulled in by all sorts of crates for the objects protos. Separate out the latter (plus StashError) into a new mz-stash-types crate that is still depended on by lots of things, but changes less often.
The various old versions of objects.proto want to be next to it, so pull them into mz-stash-types, too. In an ideal world, we'd probably move the vX_to_vX+1 upgrades also, but they depend on actual stash things (e.g. Transaction), so leave them where they are.
Finally, any given metric can only be registered once (otherwise we get a panic), but we need two stashes, so also pull out the stash Metrics into mz-stash-types. Ideally, this would have stayed where it was. A possible alternative to this would be something like putting a type-safe cache of registered metrics objects on MetricsRegistry.
Timing on my M1 laptop of bin/environmentd after invalidating stash went from 2m12s to 53s. A large part of this is likely that we've entirely eliminated clusterd, including linking.
Deps before:
Deps after:
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.