feat(mapper): add wire-size-based chunking and ResultToGetTargetGraphResponse#206
Closed
yushan8 wants to merge 6 commits into
Closed
feat(mapper): add wire-size-based chunking and ResultToGetTargetGraphResponse#206yushan8 wants to merge 6 commits into
yushan8 wants to merge 6 commits into
Conversation
…Response Introduce internal/mapper/chunk.go with BySize (generic proto chunker bounded by real serialized size) and ChunkMetadata (map splitter). Add ResultToGetTargetGraphResponse to internal/mapper/target_graph.go, converting a targethasher.Result into chunked stream responses. Both old (core/common) and new (internal/mapper) implementations coexist — callsite migration follows in a subsequent PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 15, 2026
| // ResultToGetTargetGraphResponse converts a targethasher.Result into chunked | ||
| // GetTargetGraphResponse messages ready for streaming or storage. Each message | ||
| // is bounded by maxMessageBytes of real wire size (see chunkTargets). | ||
| func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Result, maxMessageBytes int) ([]*tangopb.GetTargetGraphResponse, error) { |
Contributor
There was a problem hiding this comment.
I feel like this function is doing a lot of logic that maybe shouldn't be in mapper. This could be broken into two. First is taking targethasher.Result and processing it into optimized targets. This part can stay in orchestrator (optimized target needs an entity). Second is turning optimized targets into the proto response, so the mapper can do just this part.
Contributor
Author
There was a problem hiding this comment.
Yes good point. Going to close this in favor of #211 to split it up better.
…d proto steps Add entity.OptimizedTarget and entity.TargetGraph as proto-free domain types for target graphs. Split the monolithic ResultToGetTargetGraphResponse into: - ResultToTargetGraph: targethasher.Result → entity.TargetGraph (proto-free) - TargetGraphToProto: entity.TargetGraph → chunked proto responses This lets the orchestrator work with entity types while keeping proto conversion confined to the mapper and controller layers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…unction Callers should use the two-step flow (ResultToTargetGraph then TargetGraphToProto) directly, keeping entity and proto concerns explicitly separate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add entity.IDTarget, entity.GraphMetadata, and entity.GraphChunk as compact ID-mapped representations for streaming and storage, mirroring the proto shape without proto dependencies. Add TargetGraphToChunks (entity.TargetGraph → []entity.GraphChunk), GraphChunkToProto/ProtoToGraphChunk converters. TargetGraphToProto now composes TargetGraphToChunks + GraphChunkToProto. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Move streaming-split logic from internal/mapper/chunk.go to internal/streaming/streaming.go with clearer names: - BySize → streaming.SplitBySize (generic, proto-free) - ChunkMetadata → streaming.SplitMetadata (returns entity.GraphMetadata) The streaming package has no proto dependency — it works with the Sizer interface and entity types only. Proto conversion stays in the mapper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
yushan8
marked this pull request as draft
July 16, 2026 22:12
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
Introduces wire-size-based chunking and a proto-free domain type for target graphs, separating entity conversion from proto serialization.
entity.OptimizedTargetandentity.TargetGraphas proto-free domain types for target graph data.ResultToTargetGraph: convertstargethasher.Result→entity.TargetGraph(proto-free, suitable for orchestrator use).TargetGraphToProto: convertsentity.TargetGraph→ chunked[]*tangopb.GetTargetGraphResponse(proto serialization + ID mapping + chunking).internal/mapper/chunk.gowithBySize(generic proto chunker bounded by real serialized size viaSize()) andChunkMetadata(map splitter by measured entry wire size).BySizealways returns at least one chunk (empty input → single empty chunk); non-first empty chunks return an error.core/common) and new (internal/mapper) implementations coexist — callsite migration follows in config: bound stream chunks by real message size, not estimated #203.This split enables a future PR to have the orchestrator work with
entity.TargetGraphdirectly, removing proto from the orchestrator layer entirely.Test plan
make buildmake test—ResultToTargetGraph(entity conversion),TargetGraphToProto(empty graph),ResultToGetTargetGraphResponse(chunking at various sizes),BySize/ChunkMetadataunit testsmake gazelle && gofmt -w . && goimports -w .Stack