refactor(storage): move the gateway's four stores into submitqueue/gateway/extension/storage - #741
Conversation
…teway/extension/storage
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
|
|
||
| // 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.
This branch has not been deployed
2 participants
Why?
First half of the storage split (see the RFC in the parent PR). The gateway reaches for exactly four stores — the append-only request log and the three read models — and the orchestrator for the other nine, with no crossover in either direction, including through the shared helpers in
submitqueue/core/.What?
submitqueue/gateway/extension/storage/now holds those four store interfaces, their MySQL implementations and tests, their mocks, and their four.sqlfiles. It declares its ownFactoryand a four-accessorStorage; the old aggregate drops to nine.Two conventions this sets for the orchestrator half:
storage, so a file needing the shared errors imports the contract package asbasestorage.storage.Xtherefore always means "this service's storage".Configis a type alias (type Config = basestorage.Config), not a new type, so one config value resolves either service's storage.Also updates two repo-level lists that are invisible from the code: the
mocksMakefile target has a hardcoded package list, andqueueshardhas a hardcodedschemaRoots.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