config: bound stream chunks by real message size, not estimated#203
Open
yushan8 wants to merge 5 commits into
Open
config: bound stream chunks by real message size, not estimated#203yushan8 wants to merge 5 commits into
yushan8 wants to merge 5 commits into
Conversation
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
72aa3d7 to
68d7c4e
Compare
yushan8
changed the base branch from
yushan/config-path-suffix-rename
to
yushan/remove-undocumented-config-fields
yushan8
changed the title
yushan8
force-pushed
the
yushan/remove-undocumented-config-fields
branch
from
67934f9 to
06f02e9
Compare
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
68d7c4e to
7ee9254
Compare
yushan8
force-pushed
the
yushan/remove-undocumented-config-fields
branch
from
06f02e9 to
487e034
Compare
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
7ee9254 to
d4384e7
Compare
yushan8
force-pushed
the
yushan/remove-undocumented-config-fields
branch
from
487e034 to
adf29cc
Compare
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
9 times, most recently
from
8f25b16 to
b502b32
Compare
This was referenced Jul 15, 2026
yushan8
commented
Jul 15, 2026
| // 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)) |
Contributor
Author
There was a problem hiding this comment.
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.
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
44f8f10 to
651f382
Compare
yushan8
force-pushed
the
yushan/remove-undocumented-config-fields
branch
from
adf29cc to
2224a38
Compare
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
651f382 to
798ad49
Compare
3 tasks
yushan8
changed the base branch from
yushan/remove-undocumented-config-fields
to
yushan/mapper-chunk-methods
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
798ad49 to
5f89bbd
Compare
| b.config.Service.Streaming.MaxNumTargets, | ||
| b.config.Service.Streaming.MaxNumMetadataEntries, | ||
| ) | ||
| responses, err := mapper.ResultToGetTargetGraphResponse(ctx, result, b.config.Service.MaxMessageBytes) |
Contributor
There was a problem hiding this comment.
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?
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>
yushan8
force-pushed
the
yushan/config-max-message-bytes
branch
from
5f89bbd to
38db056
Compare
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates all callsites to the new
internal/mapperchunking primitives from #206, consolidates config, and removes the old code.service.streaming.{max_num_targets,max_num_changed_targets,max_num_metadata_entries}with a singleservice.max_message_bytesbyte budget, matchingconfig/README.md.mapper.BySize,mapper.ChunkMetadata, andmapper.TargetGraphToProto.mapper.ResultToTargetGraph(proto-free) andmapper.WriteTargetGraph(handles proto conversion + storage write internally) — the orchestrator no longer touches proto types in its write path.core/common/utils.gostripped to justToShortRemote— all chunking code, chunk constants, and proto imports removed.config.Parse()defaultsMaxMessageBytesto ~4.25 MB when unset.ChangedTarget" adjustment:ChangedTarget.Size()naturally reflects carrying two targets, so the same budget applies uniformly.Note: the orchestrator still returns
storage.GraphReader(proto-typed) and usesstorage.NewGraphReader. A follow-up PR can change the storage layer to use entity types end-to-end, fully removing the transitive proto coupling.Test plan
make buildmake testmake gazelle && gofmt -w . && goimports -w .Stack