refactor: replace proto serialization with JSON and migrate callsites by yushan8 · Pull Request #222 · uber/tango

yushan8 · 2026-07-20T03:28:30Z

Summary

Remove all proto imports from core/storage/ — serialize entity structs directly via encoding/json
Move write functions from reader files into graphwriter.go
Controller reads entity.GetTargetGraphResponse and entity.GetChangedTargetsResponse internally, converting to proto via mapper only at stream.Send
Changed-targets pipeline fully migrated to entity types
Orchestrator uses mapper.ResultToTargetGraph + mapper.ChunkTargetGraph
Consolidate service.chunking.* into a single service.max_message_bytes config field
Remove old chunking code from core/common
Update query-bench example

Test plan

grep -r "tangopb\|gogo/protobuf" core/storage/ returns zero results
make build && make test — all 19 test targets pass
make gazelle — BUILD files in sync

Stack

feat(entity): add entity types for target graph and changed targets #210 (entity types)
feat(mapper): add entity-proto mappers #211 (mappers + streaming)
refactor: replace proto serialization with JSON and migrate callsites #222 — this PR (storage JSON + callsite migration + cleanup)

…210) ## Summary - Add `entity.OptimizedTarget`, `entity.Metadata`, and `entity.GetTargetGraphResponse` as compact, proto-free representations of the target graph wire types - Add `entity.ChangedTarget` and `entity.GetChangedTargetsResponse` for the changed-targets path - These entity types mirror the proto definitions but live in a dependency-free package, preventing proto from leaking into storage, orchestrator, and other non-transport layers ## Test plan - [x] `make build && make test && make gazelle` ## Stack 1. **#210 — this PR** (entity types) 2. #211 (mappers + streaming) 3. #222 (storage JSON rewrite) 4. #212 (callsite migration + cleanup) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

## Summary - Adds entity-proto mappers for target graph and changed targets responses - `ResultToTargetGraph` — converts `targethasher.Result` into ID-mapped entity types - `GetTargetGraphResponseToProto` / `ChangedTargetsResponseToProto` — entity→proto conversions for gRPC streaming - `ProtoToGetTargetGraphRequest` — proto→entity request conversion This PR is addresses this comment to split up the Result -> Optimized target entity conversion, and OptimizedTarget -> `GetTargetGraphResponseToProto` #206. These mappers are used to avoid leaking proto definitions into internals such as storage and orchestrator. ## Stack 1. #224 (streaming split utilities) 2. **This PR** (entity-proto mappers) 3. #222 (storage JSON rewrite) 4. #212 (callsite migration) ## Test plan - [x] `make build && make test && make gazelle` --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

xytan0056 · 2026-07-21T01:03:58Z

-}
+// defaultMaxMessageBytes is the fallback max serialized size per streamed
+// message (~4.25 MB), well under the 64 MB default gRPC limit.
+const defaultMaxMessageBytes = 4_250_000


can this be exported? Multiple palces are referencing it

xytan0056 · 2026-07-21T01:04:38Z

@@ -1,4 +1,4 @@
-// Copyright (c) 2026 Uber Technologies, Inc.
+// Copyright (c) 2025 Uber Technologies, Inc.


xytan0056 · 2026-07-21T01:06:43Z

now we have internal/mapper and mapper. How about optimized?

xytan0056 · 2026-07-21T01:20:08Z

+	tempResp := mapper.ChangedTargetsResponseToProto(&entity.GetChangedTargetsResponse{ChangedTargets: changed})
+	protoChanged := tempResp.GetItem().(*pb.GetChangedTargetsResponse_ChangedTargets).ChangedTargets.GetChangedTargets()
+	var results []entity.GetChangedTargetsResponse
+	changedGroups, err := streaming.SplitBySize(protoChanged, c.maxMessageBytes)


where is streaming.SplitBySize defined? I can't find it
looks like we called ChangedTargetsResponseToProto twice here , once for splitting, once for sending later

xytan0056 · 2026-07-21T01:23:52Z

+		results = append(results, entity.GetChangedTargetsResponse{
+			ChangedTargets: changed[idx : idx+len(group)],
 		})
+		idx += len(group)


actually looks like the whole ChangedTargetsResponseToProto -> protoChanged => SplitBySize -> changedGroups is just for len(group) ? seems too expensive, can we feed something to SplitBysize without the conversion?

xytan0056

nits

- Export DefaultMaxMessageBytes in config package and use it from controller instead of hardcoded value - Update copyright year to 2026 in core/storage/reader.go - Add Size() to entity.ChangedTarget and inline size-based splitting in compareTargetGraphs, eliminating the wasteful proto conversion that was only used for size measurement - Add ProtoToChangedTargetsResponse mapper for cache read boundary Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Remove all proto from core/storage, replacing with encoding/json. Migrate controller and orchestrator to entity types, converting to proto only at the gRPC boundary. Consolidate chunking config into max_message_bytes. Remove old chunking code from core/common. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ation Update toChangeType to return entity.ChangeType, mapper to cast via int32, and test assertions to use entity constants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Run gazelle to remove stale //config and //core/common deps from controller BUILD.bazel. Replace context.Background() with t.Context() in test helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Remove _defaultMaxMessageBytes constant and inline the value with a TODO to provide it via config.Parse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

- Export DefaultMaxMessageBytes in config package and use it from controller instead of hardcoded value - Update copyright year to 2026 in core/storage/reader.go - Add Size() to entity.ChangedTarget and inline size-based splitting in compareTargetGraphs, eliminating the wasteful proto conversion that was only used for size measurement - Add ProtoToChangedTargetsResponse mapper for cache read boundary Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

yushan8 requested review from a team as code owners July 20, 2026 03:28

This was referenced Jul 20, 2026

feat(entity): add entity types for target graph and changed targets #210

Merged

feat(mapper): add entity-proto mappers #211

Merged

refactor: migrate callsites to entity types and consolidate config #212

Closed

yushan8 force-pushed the yushan/streaming-and-mappers branch from 6fc3266 to 0f5a89c Compare July 20, 2026 03:42

yushan8 force-pushed the yushan/storage-json branch 2 times, most recently from 1a54757 to 2252f29 Compare July 20, 2026 03:46

yushan8 force-pushed the yushan/streaming-and-mappers branch 2 times, most recently from 5fb83ae to 7f55e85 Compare July 20, 2026 04:52

yushan8 force-pushed the yushan/storage-json branch 2 times, most recently from 8677311 to 614cddf Compare July 20, 2026 05:27

yushan8 force-pushed the yushan/streaming-and-mappers branch from 7f55e85 to e762db8 Compare July 20, 2026 05:27

yushan8 changed the title ~~refactor(storage): replace proto serialization with JSON entity encoding~~ refactor: replace proto serialization with JSON and migrate callsites Jul 20, 2026

yushan8 force-pushed the yushan/streaming-and-mappers branch 2 times, most recently from dff47a4 to 9c060bc Compare July 20, 2026 17:17

yushan8 force-pushed the yushan/storage-json branch from 7ec0e66 to 2870453 Compare July 20, 2026 17:27

yushan8 mentioned this pull request Jul 20, 2026

feat(streaming): add split methods to split the message by maxBytes size #224

Open

1 task

yushan8 force-pushed the yushan/streaming-and-mappers branch from 9c060bc to 63d2138 Compare July 20, 2026 18:21

yushan8 force-pushed the yushan/storage-json branch 2 times, most recently from 140a936 to 1f1b327 Compare July 20, 2026 18:33

yushan8 force-pushed the yushan/streaming-and-mappers branch from 63d2138 to 7b87081 Compare July 20, 2026 18:41

yushan8 force-pushed the yushan/storage-json branch from 1f1b327 to 6d0f3a7 Compare July 20, 2026 18:42

yushan8 force-pushed the yushan/streaming-and-mappers branch from 7b87081 to 23bf3ac Compare July 20, 2026 19:32

yushan8 force-pushed the yushan/storage-json branch 3 times, most recently from e57a2e1 to 1f8366f Compare July 20, 2026 21:47

yushan8 force-pushed the yushan/streaming-and-mappers branch from e762709 to 8f360d1 Compare July 20, 2026 22:09

yushan8 force-pushed the yushan/storage-json branch from 1f8366f to e9517ce Compare July 20, 2026 22:13

yushan8 force-pushed the yushan/streaming-and-mappers branch from 8f360d1 to 15fe2f7 Compare July 20, 2026 22:20

yushan8 force-pushed the yushan/storage-json branch from e9517ce to af13970 Compare July 20, 2026 22:21

yushan8 force-pushed the yushan/streaming-and-mappers branch from 15fe2f7 to 814e971 Compare July 20, 2026 22:32

yushan8 force-pushed the yushan/storage-json branch from af13970 to cb19db6 Compare July 20, 2026 22:34

Base automatically changed from yushan/streaming-and-mappers to yushan/streaming-split July 20, 2026 23:36

xytan0056 reviewed Jul 21, 2026

View reviewed changes

Comment thread mapper/mapper.go

xytan0056 Jul 21, 2026

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now we have internal/mapper and mapper. How about optimized?

xytan0056 reviewed Jul 21, 2026

View reviewed changes

yushan8 and others added 5 commits July 20, 2026 21:39

refactor: use entity.ChangeType instead of int32 for change classific…

f2aa360

…ation Update toChangeType to return entity.ChangeType, mapper to cast via int32, and test assertions to use entity constants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

fix(controller): update BUILD deps and use t.Context() in tests

c47b95c

Run gazelle to remove stale //config and //core/common deps from controller BUILD.bazel. Replace context.Background() with t.Context() in test helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

refactor(controller): inline default max message bytes

dbae9bc

Remove _defaultMaxMessageBytes constant and inline the value with a TODO to provide it via config.Parse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

yushan8 force-pushed the yushan/storage-json branch from 28a89e0 to 4860e6e Compare July 21, 2026 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: replace proto serialization with JSON and migrate callsites#222

refactor: replace proto serialization with JSON and migrate callsites#222
yushan8 wants to merge 5 commits into
yushan/streaming-splitfrom
yushan/storage-json

yushan8 commented Jul 20, 2026 •

edited

Loading

Uh oh!

xytan0056 Jul 21, 2026 •

edited

Loading

Uh oh!

xytan0056 Jul 21, 2026

Uh oh!

xytan0056 Jul 21, 2026

Uh oh!

xytan0056 Jul 21, 2026

Uh oh!

xytan0056 Jul 21, 2026

Uh oh!

xytan0056 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,4 +1,4 @@
		// Copyright (c) 2026 Uber Technologies, Inc.
		// Copyright (c) 2025 Uber Technologies, Inc.

Uh oh!

Conversation

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

Uh oh!

Summary

Test plan

Stack

Uh oh!

xytan0056 Jul 21, 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!

xytan0056 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

xytan0056 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

xytan0056 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

xytan0056 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

xytan0056 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yushan8 commented Jul 20, 2026 •

edited

Loading

xytan0056 Jul 21, 2026 •

edited

Loading