config: bound stream chunks by real message size, not estimated by yushan8 · Pull Request #203 · uber/tango

yushan8 · 2026-07-14T20:49:18Z

Summary

Migrates all callsites to the new internal/mapper chunking primitives from #206, consolidates config, removes old code, and replaces proto-typed storage interfaces with entity-typed ones.

Replaces service.streaming.{max_num_targets,max_num_changed_targets,max_num_metadata_entries} with a single service.max_message_bytes byte budget, matching config/README.md.
Introduces mapper.GraphChunkReader (wraps storage.GraphReader, converts proto → entity.GraphChunk on read) and mapper.WriteTargetGraph (converts entity.TargetGraph → proto, writes to storage).
Orchestrator.GetTargetGraph now returns *mapper.GraphChunkReader instead of storage.GraphReader — the orchestrator no longer touches proto types.
Controller reads entity.GraphChunk from the reader and converts to proto at stream-send time via mapper.GraphChunkToProto.
core/common/utils.go stripped to just ToShortRemote — all chunking code, chunk constants, and proto imports removed.
config.Parse() defaults MaxMessageBytes to ~4.25 MB when unset.

Test plan

make build
make test — 19/19 pass
make gazelle && gofmt -w . && goimports -w .

Stack

yushan8 · 2026-07-15T04:42:26Z

+// messages, each bounded by maxMessageBytes of real wire size.
+func chunkTargets(targets []*tangopb.OptimizedTarget, maxMessageBytes int) []*tangopb.GetTargetGraphResponse {
+	chunks := BySize(targets, maxMessageBytes)
+	responses := make([]*tangopb.GetTargetGraphResponse, 0, len(chunks))


Moved chunkTargets to internal/mapper to avoid leaking proto definitions into other packages. We also want to utilize proto's built-in Size() to calculate the size of the message we're sending over the stream.

justinwon777 · 2026-07-16T00:21:26Z

-		b.config.Service.Streaming.MaxNumTargets,
-		b.config.Service.Streaming.MaxNumMetadataEntries,
-	)
+	responses, err := mapper.ResultToGetTargetGraphResponse(ctx, result, b.config.Service.MaxMessageBytes)


this is bringing in proto to orchestrator which is what we're trying to prevent? Can storage.WriteGraphStream input not be the proto response so we can get rid of having to map result to proto here?

Good idea, going to close this and refactor that out here #212

Replaces service.streaming.{max_num_targets,max_num_changed_targets, max_num_metadata_entries} with a single service.max_message_bytes byte budget, matching config/README.md. The three separate counts all bounded the same underlying thing (gRPC message size) and it wasn't obvious to an operator which one to tune. config.ChunkSizesForByteBudget converts the budget into the three internal per-entry-type limits once, at wiring time, using fixed size assumptions (a target ~40KB worst case, a changed target ~2x that, a metadata entry ~85 bytes) -- not by measuring real messages per request, so there's no added per-request cost. Default of 4,250,000 bytes matches today's metadata chunk size exactly (50,000 x ~85 bytes). The byte-budget conversion lives in the config package (config-specific concern), not core/common, which keeps only its own small, independent fallback constants for chunkTargets/ChunkMetadata's <=0 safety net.

Bound each streamed message by its actual serialized size (via each message type's generated Size() method) rather than converting max_message_bytes into a fixed per-entry-type chunk count using a guessed average entry size. Introduces internal/msgsplit for all chunking logic (BySize, ChunkMetadata, ResultToGetTargetGraphResponse, DefaultMaxMessageBytes), moving it out of core/common which was accumulating unrelated responsibilities. core/common retains only ToShortRemote and NameIDMapper.

BySize now always returns at least one chunk (empty input yields a single empty chunk) so callers always have a message to send on the stream. Non-first chunks that are empty return an error since that indicates a bug in the splitting logic. Same contract for ChunkMetadata. Removes the per-type wrapper functions (chunkTargets, ChunkChangedTargets) — callers inline the one-liner proto wrapping themselves, keeping BySize and ChunkMetadata as the two public APIs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Add mapper.WriteTargetGraph which accepts entity.TargetGraph and handles proto conversion + storage write internally. The orchestrator now only touches entity types when writing computed graphs — proto conversion is confined to the mapper layer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Update query-bench to use the explicit two-step flow now that the ResultToGetTargetGraphResponse convenience function has been removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…nd controller Replace storage.GraphReader (proto-typed) with mapper.GraphChunkReader (entity-typed) in the Orchestrator interface, native orchestrator, and controller. Proto conversion now happens at stream-send time in the controller via GraphChunkToProto, keeping the orchestrator proto-free. Add mapper.NewGraphChunkReader (wraps storage.NewGraphReader, converts proto → entity on read) and mapper.ReadAllGraphChunks for batch reads. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Update getchangedtargets to use the new streaming package for splitting changed targets and metadata, replacing the removed mapper.BySize and mapper.ChunkMetadata. Export EntityMetadataToProto for the entity → proto metadata conversion at the controller boundary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

yushan8 requested review from a team as code owners July 14, 2026 20:49

yushan8 marked this pull request as draft July 14, 2026 21:41

yushan8 force-pushed the yushan/config-max-message-bytes branch from 72aa3d7 to 68d7c4e Compare July 14, 2026 22:19

yushan8 changed the base branch from yushan/config-path-suffix-rename to yushan/remove-undocumented-config-fields July 14, 2026 22:20

yushan8 changed the title ~~config: consolidate streaming chunk-size fields into max_message_bytes~~ config: bound stream chunks by real message size, not estimated Jul 14, 2026

yushan8 force-pushed the yushan/remove-undocumented-config-fields branch from 67934f9 to 06f02e9 Compare July 14, 2026 23:10

yushan8 force-pushed the yushan/config-max-message-bytes branch from 68d7c4e to 7ee9254 Compare July 14, 2026 23:13

yushan8 force-pushed the yushan/remove-undocumented-config-fields branch from 06f02e9 to 487e034 Compare July 14, 2026 23:19

yushan8 force-pushed the yushan/config-max-message-bytes branch from 7ee9254 to d4384e7 Compare July 14, 2026 23:20

yushan8 force-pushed the yushan/remove-undocumented-config-fields branch from 487e034 to adf29cc Compare July 14, 2026 23:40

yushan8 force-pushed the yushan/config-max-message-bytes branch 9 times, most recently from 8f25b16 to b502b32 Compare July 15, 2026 04:22

This was referenced Jul 15, 2026

[config] Tango configuration documentation #180

Open

config: update fields, defaults, and required checks to match README #190

Open

config: remove fields not documented in config/README.md #191

Open

yushan8 commented Jul 15, 2026

View reviewed changes

yushan8 force-pushed the yushan/config-max-message-bytes branch from 44f8f10 to 651f382 Compare July 15, 2026 17:36

yushan8 force-pushed the yushan/remove-undocumented-config-fields branch from adf29cc to 2224a38 Compare July 15, 2026 17:44

yushan8 force-pushed the yushan/config-max-message-bytes branch from 651f382 to 798ad49 Compare July 15, 2026 17:59

yushan8 mentioned this pull request Jul 15, 2026

feat(mapper): add wire-size-based chunking and ResultToGetTargetGraphResponse #206

Closed

3 tasks

yushan8 changed the base branch from yushan/remove-undocumented-config-fields to yushan/mapper-chunk-methods July 15, 2026 17:59

yushan8 force-pushed the yushan/config-max-message-bytes branch from 798ad49 to 5f89bbd Compare July 15, 2026 18:06

yushan8 marked this pull request as ready for review July 15, 2026 18:13

justinwon777 reviewed Jul 16, 2026

View reviewed changes

yushan8 force-pushed the yushan/config-max-message-bytes branch 3 times, most recently from 657ee52 to 0579493 Compare July 16, 2026 20:49

yushan8 and others added 7 commits July 16, 2026 15:09

Update

d3f0dee

refactor(example): use two-step ResultToTargetGraph + TargetGraphToProto

dd4e0a3

Update query-bench to use the explicit two-step flow now that the ResultToGetTargetGraphResponse convenience function has been removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

yushan8 marked this pull request as draft July 16, 2026 22:12

yushan8 force-pushed the yushan/config-max-message-bytes branch from 0579493 to e1d5cf6 Compare July 16, 2026 22:12

yushan8 closed this Jul 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

config: bound stream chunks by real message size, not estimated#203

config: bound stream chunks by real message size, not estimated#203
yushan8 wants to merge 8 commits into
yushan/mapper-chunk-methodsfrom
yushan/config-max-message-bytes

yushan8 commented Jul 14, 2026 •

edited

Loading

Uh oh!

yushan8 Jul 15, 2026

Uh oh!

justinwon777 Jul 16, 2026 •

edited

Loading

Uh oh!

yushan8 Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

yushan8 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Stack

Uh oh!

yushan8 Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

justinwon777 Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yushan8 Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yushan8 commented Jul 14, 2026 •

edited

Loading

justinwon777 Jul 16, 2026 •

edited

Loading