*: split out {controller,compute,storage}-types crates #21554

danhhz · 2023-09-01T23:41:29Z

This allows changes to implementation code to invalidate fewer crates, hopefully improving dev iteration times.

Notably, mz-sql and mz-transform (both heavyweight) no longer depend on storage-client. As mentioned in the reviewer notes, we could easily add mz-compute to that set.

Real-world timings:

touch src/storage-client/src/lib.rs; cargo check

Before: 9.25s
After: 8.08s

touch src/storage-client/src/lib.rs; cargo build -p mz-environmentd -p mz-clusterd --lib

Before: 45.29s
After: 22.41s
This is synthetic for two reasons. First, I added an empty file at src/clusterd/src/lib.rs to make --lib work. Second, as the next timings show, link times still dominate.

touch src/storage-client/src/lib.rs; cargo build --bin environmentd --bin clusterd

Before: 1m40s
After: 1m39s

Deps above mz-storage-client (before)

Deps above mz-storage-client (after)

Motivation

This PR refactors existing code.

Tips for reviewer

Opening as a draft because there's some things to work out. Also, I got a touch sloppy with the commit hygiene and import blocks, I'll go back and tighten that up if we decide that this is desirable and if/when the final names are decided.

I feel pretty good about the foo-types model:

There is already a mz-persist-types crate with exactly this sort of thing.
Both mz-storage-client and mz-compute-client had types modules, which is mostly what got moved in.
At a previous job with much worse compile times (scala), we ended up very successfully using a foo-types, foo-service (interface), foo-concrete model. foo-service corresponds to our mz-foo-client and foo-concrete corresponds to our mz-foo.

Names that are more or less a strawman at this point:

I split out an mz-storage-operators to hold persist_source because it was the only reason mz-compute was depending on mz-storage-client (and we don't want to move it into mz-storage because mz-compute does not depend on mz-storage).
As written this PR currently moves the storage controller trait out from mz-storage-client into mz-storage-types, but I think that's wrong. Instead, we should move the storage controller trait impl into a new crate, something like mz-storage-controller.

Intuition for the various names and what should live in them:

mz-storage-types, mz-compute-types: POD (plain old data), protos, no business logic (in reality, minimal business logic)
mz-storage-client, mz-compute-client: The adaptor APIs to storage and compute (aka the controllers)
mz-storage-operators: The compute API to storage
mz-storage, mz-compute: Storage and compute dataflows

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

danhhz · 2023-09-01T23:43:00Z

@benesch See "Tips for reviewer" for a bunch of context/questions. If we decide we like the idea and once we hash out names, I'd also love to hear your thoughts on which stakeholders I should pull in from the individual teams.

benesch · 2023-09-02T03:43:45Z

I really like the look of the "after" diagram. Unfortunately I'm not going to have time to look at the specifics of this myself! Will follow up on Slack about alternative reviewers.

danhhz · 2023-09-05T18:57:00Z

@antiguru @teskje @petrosagg Added y'all as individual reviewers on behalf of compute and storage. Let me know if someone else would be more appropriate. This is marked as a draft since I have some open questions (see "Tips for reviewer"), but the concept is ready for review.

Happy to answer questions, provide context, etc! I'd love to get signoffs first such that I only have to do the rebase once to land it. :)

antiguru

Looks great! Looked through all files, nothing stood out.

antiguru · 2023-09-05T20:29:54Z

src/adapter/src/coord/sequencer/linked_cluster.rs

-    DEFAULT_REPLICA_LOGGING_INTERVAL_MICROS,
-};
+use mz_controller::clusters::{ReplicaAllocation, ReplicaConfig, ReplicaLogging};
+use mz_controller_types::{ClusterId, DEFAULT_REPLICA_LOGGING_INTERVAL_MICROS};


~~DEFAULT_REPLICA_LOGGING_INTERVAL_MICROS~~

I was wondering whether we could move this elsewhere, but came to no conclusion!

This const is itself a pub use of mz_compute_client::controller::DEFAULT_COMPUTE_REPLICA_LOGGING_INTERVAL_MICROS (moved to mz_compute_types in this PR), which seemed intentional, so I left it alone.

Happy to e.g. remove the re-export and inline it if you think that's better?

It is weird that the controller defines a constant that's only used by the adapter and sql crates. Seems like it should be defined in one of those instead?

teskje

Compute changes look reasonable to me! I had a couple thoughts, but it's fine to leave them as follow ups for the compute team.

I'm wondering if there is anything we can do to be more aware of crate interdependencies in the future. I think many of us don't spend a lot of thought about adding new dependencies if convenient, and I worry that we might slowly revert to the old spiderweb if we are not careful about it.

teskje · 2023-09-06T11:45:13Z

src/clusterd/src/lib.rs

Just a reminder so you don't forget to remove this again!

Oh whoops! Never even meant to add this to a commit. Done

teskje · 2023-09-06T11:47:52Z

src/compute-client/src/controller.rs


 mod instance;
 mod replica;

 pub mod error;

-/// Identifier of a compute instance.
-pub type ComputeInstanceId = StorageInstanceId;
-
 /// Identifier of a replica.
 pub type ReplicaId = mz_cluster_client::ReplicaId;


Can we also move ReplicaId into mz_compute_types? It's a bit weird that this is now defined in a different place than ComputeInstanceId.

Although... thinking about it, we should probably just remove this pub type entirely and just use mz_cluster_client::ReplicaId everywhere.

Happy to do either. Pushed the latter

teskje · 2023-09-06T11:50:04Z

src/compute-client/src/protocol/response.proto

@@ -23,7 +22,7 @@ message ProtoComputeResponse {
    message ProtoPeekResponseKind {
        mz_proto.ProtoU128 id = 1;
        ProtoPeekResponse resp = 2;
-    map<string, string> otel_ctx = 3;
+        map<string, string> otel_ctx = 3;


teskje · 2023-09-06T11:53:17Z

src/compute-client/src/protocol/response.proto

        ProtoPeekResponseKind peek_response = 2;
        ProtoSubscribeResponseKind subscribe_response = 3;
    }
 }

+message ProtoTrace {


I have never understood why this is called "trace". Maybe this is an opportunity to rename it to ProtoUpper or somesuch? Don't feel obligated if you think that's out of scope!

As I mentioned in the review notes, I suspect we'll end up discarding this commit entirely and doing something else, but if it sticks around happy to do this rename.

danhhz

I'm wondering if there is anything we can do to be more aware of crate interdependencies in the future. I think many of us don't spend a lot of thought about adding new dependencies if convenient, and I worry that we might slowly revert to the old spiderweb if we are not careful about it.

I'd love to add a lint that prevents certain deps, but that's definitely something I'll have to leave for future skunkworks.

danhhz · 2023-09-06T14:37:27Z

src/clusterd/src/lib.rs

Oh whoops! Never even meant to add this to a commit. Done

danhhz · 2023-09-06T14:40:16Z

src/compute-client/src/protocol/response.proto

        ProtoPeekResponseKind peek_response = 2;
        ProtoSubscribeResponseKind subscribe_response = 3;
    }
 }

+message ProtoTrace {


As I mentioned in the review notes, I suspect we'll end up discarding this commit entirely and doing something else, but if it sticks around happy to do this rename.

danhhz · 2023-09-06T14:54:10Z

src/compute-client/src/controller.rs


 mod instance;
 mod replica;

 pub mod error;

-/// Identifier of a compute instance.
-pub type ComputeInstanceId = StorageInstanceId;
-
 /// Identifier of a replica.
 pub type ReplicaId = mz_cluster_client::ReplicaId;


Happy to do either. Pushed the latter

petrosagg

The storage-types split looks good! On storage-operators, is that crate pulling its weight? Is the idea behind it that it allows iterating on persist_source without recompiling mz-storage, which is slow?

If yes, we can also go the other way and keep persist_source in mz-storage but split out each source to its own crate. Sources already implement a trait so that ought to make things a bit better because each per-source crate will only be recompiled when that particular source changes.

Not a strong opinion on this so if you feel strongly about mz-storage-operators go ahead with your idea as-is. I'll experiment with splitting sources out in some future skunkworks

danhhz · 2023-09-08T14:58:37Z

On storage-operators, is that crate pulling its weight? Is the idea behind it that it allows iterating on persist_source without recompiling mz-storage, which is slow?

Actually, it's almost the opposite in a way. The idea is that it allows iterating on mz-storage-client without invalidating mz-compute.

The persist_source commit is easily the least invasive of any of these, so happy to leave it off this PR, discuss async, and do whatever we decide in a followup if you prefer?

petrosagg · 2023-09-08T15:58:02Z

I see. It sounds like mz_compute should depend on mz_storage, just like mz_compute_client depends on mz_storage_client but that may impact linking times. Let's leave the mz_storage_operators as you have it!

Removes dep: - mz-sql -> mz-controller

Removes deps: - mz-transform -> mz-compute-client - mz-sql -> mz-compute-client - mz-controller-types -> mz-compute-client

Removes deps: - mz-sql -> mz-storage-client - sqllogictest -> mz-storage-client - mz-compute-types -> mz-storage-client

Removes deps: - mz-cluster -> mz-compute-client - mz-cluster -> mz-storage-client

Removes dep: - mz-compute -> mz-storage-operators - mz-storage-client -> mz-expr

danhhz · 2023-09-10T21:24:04Z

TFTRs, everyone!!

I played locally with splitting the storage controller impl out into a new crate instead of splitting the trait out into mz-storage-types, and I do think it's better, but it's non-obvious enough that it's worth going through a round of review. I'm going to pull that out into its own PR so I can go ahead and get this one merged

shepherdlybot · 2023-09-10T21:24:39Z

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?

Risk Score	Probability	Buggy File Hotspots
🔴 88 / 100	72%	20

Buggy File Hotspots:

File	Percentile
../coord/sequencer.rs	93
../catalog/builtin_table_updates.rs	96
../src/catalog.rs	99
../src/client.rs	93
../render/sources.rs	95
../src/rbac.rs	98
../src/controller.rs	95
../catalog/storage.rs	90
../session/vars.rs	91
../src/coord.rs	99
../src/plan.rs	92
../src/clusters.rs	92
../sequencer/inner.rs	100
../render/persist_sink.rs	91
../src/session.rs	94
../src/pure.rs	96
../src/storage_state.rs	90
../src/server.rs	94
../controller/rehydration.rs	92
../src/compute_state.rs	95

A bunch of these crate deps were recently removed in MaterializeInc#21554 in an effort to improve development iteration times (by creating fewer invalidations when changing commonly worked on crates). Keep them from accidentally coming back by adding a CI lint.

danhhz requested a review from benesch September 1, 2023 23:41

danhhz requested review from antiguru, petrosagg and teskje and removed request for benesch September 5, 2023 18:53

antiguru approved these changes Sep 5, 2023

View reviewed changes

teskje approved these changes Sep 6, 2023

View reviewed changes

danhhz commented Sep 6, 2023

View reviewed changes

petrosagg approved these changes Sep 8, 2023

View reviewed changes

controller: split out a controller-types crate

e409827

Removes dep: - mz-sql -> mz-controller

danhhz force-pushed the deps branch from 250577f to c0704db Compare September 10, 2023 19:13

danhhz added 5 commits September 10, 2023 13:50

compute: split out a compute-types crate

a94cc35

Removes deps: - mz-transform -> mz-compute-client - mz-sql -> mz-compute-client - mz-controller-types -> mz-compute-client

storage: split out a storage-types crate

96911a8

Removes deps: - mz-sql -> mz-storage-client - sqllogictest -> mz-storage-client - mz-compute-types -> mz-storage-client

cluster: move TryIntoTimelyConfig trait into mz_cluster_client

f02d394

Removes deps: - mz-cluster -> mz-compute-client - mz-cluster -> mz-storage-client

storage: split out a storage-operators crate

d3d83f0

Removes dep: - mz-compute -> mz-storage-operators - mz-storage-client -> mz-expr

cluster: remove unused mz-expr dep

9cdb6ef

danhhz force-pushed the deps branch from b455da3 to 9cdb6ef Compare September 10, 2023 20:50

danhhz marked this pull request as ready for review September 10, 2023 21:24

danhhz requested review from a team September 10, 2023 21:24

danhhz requested a review from a team as a code owner September 10, 2023 21:24

danhhz requested a review from a team September 10, 2023 21:24

danhhz requested review from a team and benesch as code owners September 10, 2023 21:24

danhhz requested a review from andrewrodriguez-m September 10, 2023 21:24

danhhz merged commit 2eff0e0 into MaterializeInc:main Sep 10, 2023
1 check passed

danhhz deleted the deps branch September 10, 2023 21:24

danhhz mentioned this pull request Sep 11, 2023

ci: add crate dependency linting #21696

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: split out {controller,compute,storage}-types crates #21554

*: split out {controller,compute,storage}-types crates #21554

danhhz commented Sep 1, 2023 •

edited

Loading

danhhz commented Sep 1, 2023

benesch commented Sep 2, 2023

danhhz commented Sep 5, 2023

antiguru left a comment

antiguru Sep 5, 2023 •

edited

Loading

danhhz Sep 5, 2023

teskje Sep 6, 2023

teskje left a comment

teskje Sep 6, 2023

danhhz Sep 6, 2023

teskje Sep 6, 2023

danhhz Sep 6, 2023

teskje Sep 6, 2023

teskje Sep 6, 2023

danhhz Sep 6, 2023

danhhz left a comment

danhhz Sep 6, 2023

danhhz Sep 6, 2023

danhhz Sep 6, 2023

petrosagg left a comment

danhhz commented Sep 8, 2023

petrosagg commented Sep 8, 2023

danhhz commented Sep 10, 2023

shepherdlybot bot commented Sep 10, 2023

*: split out {controller,compute,storage}-types crates #21554

*: split out {controller,compute,storage}-types crates #21554

Conversation

danhhz commented Sep 1, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

danhhz commented Sep 1, 2023

benesch commented Sep 2, 2023

danhhz commented Sep 5, 2023

antiguru left a comment

Choose a reason for hiding this comment

antiguru Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teskje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhhz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrosagg left a comment

Choose a reason for hiding this comment

danhhz commented Sep 8, 2023

petrosagg commented Sep 8, 2023

danhhz commented Sep 10, 2023

shepherdlybot bot commented Sep 10, 2023

danhhz commented Sep 1, 2023 •

edited

Loading

antiguru Sep 5, 2023 •

edited

Loading