refactor(storage): move the gateway's four stores into submitqueue/gateway/extension/storage - #741
Conversation
behinddwalls
left a comment
behinddwalls
left a comment
There was a problem hiding this comment.
Agent-written.
The service aggregate/schema split is right. These comments cover the remaining differences from the proposed structure.
| // the binding are rejected, and reads never surface another queue's records. | ||
| type Storage interface { | ||
| // GetRequestLogStore returns the RequestLogStore instance. | ||
| GetRequestLogStore() RequestLogStore |
There was a problem hiding this comment.
Agent-written.
Keep store interfaces in submitqueue/extension/storage/; move only the service aggregate, implementation, mocks, and schema. The 4-accessor gateway Storage already enforces the boundary.
| "github.com/uber/submitqueue/submitqueue/entity" | ||
| "github.com/uber/submitqueue/submitqueue/extension/storage" | ||
| basestorage "github.com/uber/submitqueue/submitqueue/extension/storage" | ||
| storage "github.com/uber/submitqueue/submitqueue/gateway/extension/storage" |
There was a problem hiding this comment.
Agent-written.
This is the dependency inversion caused by moving the interfaces: domain core/ now imports a service package. Keeping the interfaces in the domain storage contract avoids it.
There was a problem hiding this comment.
But even if we keep the store interfaces in domain level, Materializer still takes Factory and resolves per queue (m.stores.For(...{QueueName: log.Queue})), so it imports gateway/extension/storage either way. Same for terminate.go, batch/{find,list,transition}.go and changeset/resolver.go, which also take aggregates. I don't think we can actually avoid importing a service package here
There was a problem hiding this comment.
i guess the question is? should this also be moved to gateway now?
There was a problem hiding this comment.
Technically yes. This is the last section of the RFC (#740). I'd raised it there as a follow-on for a later stage and keep this stack focusing on the schema refactor. Happy to fold it in here if you like.
|
|
||
| // Apply schemas programmatically to application database | ||
| testutil.ApplySchema(t, s.log, s.db, testutil.SchemaDir("submitqueue/extension/storage/mysql/schema")) | ||
| testutil.ApplySchema(t, s.log, s.db, testutil.SchemaDir("submitqueue/gateway/extension/storage/mysql/schema")) |
There was a problem hiding this comment.
Agent-written.
The orchestrator suite should not provision gateway tables. Removing this is a useful test that the 9-store service boundary is complete.
| @@ -0,0 +1,5 @@ | |||
| filegroup( | |||
| name = "schema", | |||
| srcs = glob(["*.sql"]), | |||
There was a problem hiding this comment.
Agent-written.
Please also expose a union schema target for colocated deployments. Separate service schemas are opt-in; e2e/local should not need to know every service schema directory.
behinddwalls
left a comment
behinddwalls
left a comment
There was a problem hiding this comment.
Agent-written.
Requesting changes: the 4-accessor gateway aggregate and schema split are right, but store interfaces should stay in submitqueue/extension/storage/. core/ importing gateway storage is the inversion to avoid. Also drop gateway schema from the orchestrator suite and add a union schema target for colocated deploys.
| "github.com/uber/submitqueue/submitqueue/entity" | ||
| "github.com/uber/submitqueue/submitqueue/extension/storage" | ||
| basestorage "github.com/uber/submitqueue/submitqueue/extension/storage" | ||
| storage "github.com/uber/submitqueue/submitqueue/gateway/extension/storage" |
There was a problem hiding this comment.
same for this? should this be in gateway now?
There was a problem hiding this comment.
Same as Materializer
e1b1cc8 to
d13b22b
Compare
d13b22b to
52bb2aa
Compare
52bb2aa to
5806046
Compare
…teway/extension/storage
5806046 to
d638ec2
Compare
This branch has not been deployed
2 participants
Why?
First half of the split. The gateway resolves exactly four stores — the append-only request log and the three read models — and the orchestrator the other nine, with no overlap in either direction.
What?
submitqueue/gateway/extension/storage/gains its ownFactoryand a four-accessorStorage, plus the four MySQL implementations, their tests and their four.sqlfiles. The old aggregate drops to nine accessors.The thirteen store contracts and their mocks stay at
submitqueue/extension/storage/. That package keeps its import path, so callers naming only a contract or an error are untouched.Also adds a union schema target (
//submitqueue/extension/storage:schema) plus atestutilhelper, so colocated callers name no service directory, and narrows each per-service integration suite to its own schema.Two repo-level lists needed the new package, neither discoverable from the code: the
mocksMakefile target andqueueshard'sschemaRoots.Test Plan
go build ./...,go vet ./...,make lint,make check-mocks,make check-gazelle,make check-tidyall clean;make testpasses 121/121.queueshardstill reports 25 tables.Issue