From ae5861c05cd9a667b7794f19880cd7943adbf870 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 10 Jul 2026 11:39:26 -0700 Subject: [PATCH 1/6] Make sure config values are actually being used not defaults --- core/common/utils.go | 18 ++++++------------ core/common/utils_test.go | 2 +- example/cmd/query-bench/main.go | 2 +- orchestrator/native_orchestrator.go | 5 ++++- orchestrator/testdata/config.yaml | 4 ++++ 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/core/common/utils.go b/core/common/utils.go index 90df8021..e4487582 100644 --- a/core/common/utils.go +++ b/core/common/utils.go @@ -137,8 +137,10 @@ func HashRequestOptions(opts *tangopb.RequestOptions) string { // for typical target rates. const cancelCheckInterval = 4096 -// ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse -func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Result) ([]*tangopb.GetTargetGraphResponse, error) { +// ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse. +// targetChunkSize controls how many OptimizedTarget entries per stream message. +// metadataMapChunkSize controls how many entries per metadata map chunk. +func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Result, targetChunkSize, metadataMapChunkSize int) ([]*tangopb.GetTargetGraphResponse, error) { // Map target names to ids. This list is topologically sorted, so the ids are stable. // IDs start at 1 — 0 is reserved as the proto3 "unset" sentinel so consumers using // encoding/json (which honors `omitempty` on int32 fields) never silently lose a target. @@ -224,14 +226,14 @@ func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Res attrStrValIDToVal := attrStrValMapper.Invert() // chunk targets into multiple messages for streaming - responses := chunkTargets(optimizedTargets, DefaultTargetChunkSize) + responses := chunkTargets(optimizedTargets, targetChunkSize) for _, meta := range ChunkMetadata( targetIDToName, ruleTypeIDToName, tagIDToName, attrNameIDToName, attrStrValIDToVal, - DefaultMetadataMapChunkSize, + metadataMapChunkSize, ) { responses = append(responses, &tangopb.GetTargetGraphResponse{ Item: &tangopb.GetTargetGraphResponse_Metadata{Metadata: meta}, @@ -242,10 +244,6 @@ func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Res } func chunkTargets(targets []*tangopb.OptimizedTarget, chunkSize int) []*tangopb.GetTargetGraphResponse { - if chunkSize <= 0 { - chunkSize = DefaultTargetChunkSize - } - // at least one chunk numChunks := max(1, (len(targets)+chunkSize-1)/chunkSize) @@ -293,10 +291,6 @@ func ChunkMetadata( attrStrValIDToVal map[int32]string, chunkSize int, ) []*tangopb.Metadata { - if chunkSize <= 0 { - chunkSize = DefaultMetadataMapChunkSize - } - targetChunks := splitMap(targetIDToName, chunkSize) attrValChunks := splitMap(attrStrValIDToVal, chunkSize) diff --git a/core/common/utils_test.go b/core/common/utils_test.go index 4172da07..41847f19 100644 --- a/core/common/utils_test.go +++ b/core/common/utils_test.go @@ -198,7 +198,7 @@ func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { result.Targets[name] = &targethasher.Target{Name: name, Hash: []byte{0}, RuleType: "go_library"} } - responses, err := ResultToGetTargetGraphResponse(context.Background(), result) + responses, err := ResultToGetTargetGraphResponse(context.Background(), result, 250, 50_000) require.NoError(t, err) // 2 target chunks + 1 metadata chunk (500 targets well under DefaultMetadataMapChunkSize) diff --git a/example/cmd/query-bench/main.go b/example/cmd/query-bench/main.go index 3071b018..b3504605 100644 --- a/example/cmd/query-bench/main.go +++ b/example/cmd/query-bench/main.go @@ -112,7 +112,7 @@ func run() error { totalDuration += elapsed fmt.Printf("run %d: targethasher: %v (%d targets)\n", i+1, elapsed.Round(time.Millisecond), len(targethasherResult.TargetNames)) start = time.Now() - response, err := common.ResultToGetTargetGraphResponse(ctx, targethasherResult) // also print the time to convert to GetTargetGraphResponse format + response, err := common.ResultToGetTargetGraphResponse(ctx, targethasherResult, common.DefaultTargetChunkSize, common.DefaultMetadataMapChunkSize) // also print the time to convert to GetTargetGraphResponse format if err != nil { return fmt.Errorf("run %d: converting to GetTargetGraphResponse: %w", i+1, err) } diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index dda336e2..e84241ef 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -206,7 +206,10 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget if err != nil { return nil, fmt.Errorf("compute target graph: %w", err) } - responses, err := common.ResultToGetTargetGraphResponse(ctx, result) + responses, err := common.ResultToGetTargetGraphResponse(ctx, result, + b.config.Service.Chunking.TargetChunkSize, + b.config.Service.Chunking.MetadataMapChunkSize, + ) if err != nil { return nil, fmt.Errorf("convert target graph to response: %w", err) } diff --git a/orchestrator/testdata/config.yaml b/orchestrator/testdata/config.yaml index f420f74f..34887e08 100644 --- a/orchestrator/testdata/config.yaml +++ b/orchestrator/testdata/config.yaml @@ -11,3 +11,7 @@ repository: service: worker_pool_size: 3 + chunking: + target_chunk_size: 250 + changed_target_chunk_size: 125 + metadata_map_chunk_size: 50000 From bdbca68909031bc48c37f4fcc21b13ef68abbc80 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 10 Jul 2026 12:10:23 -0700 Subject: [PATCH 2/6] Update defaults --- core/common/utils_test.go | 50 +++++++++++++++++++++++++-------- example/cmd/query-bench/main.go | 5 +++- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/core/common/utils_test.go b/core/common/utils_test.go index 41847f19..df9d5b7e 100644 --- a/core/common/utils_test.go +++ b/core/common/utils_test.go @@ -186,7 +186,6 @@ func TestChunkTargets(t *testing.T) { func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { t.Parallel() - // 500 targets with DefaultTargetChunkSize=250 → 2 target chunks + 1 metadata = 3 responses numTargets := 500 result := targethasher.Result{ TargetNames: make([]string, numTargets), @@ -198,16 +197,45 @@ func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { result.Targets[name] = &targethasher.Target{Name: name, Hash: []byte{0}, RuleType: "go_library"} } - responses, err := ResultToGetTargetGraphResponse(context.Background(), result, 250, 50_000) - require.NoError(t, err) - - // 2 target chunks + 1 metadata chunk (500 targets well under DefaultMetadataMapChunkSize) - require.Len(t, responses, 3) + tests := []struct { + name string + targetChunkSize int + metadataMapChunkSize int + wantTargetChunks int + }{ + { + name: "250 per chunk", + targetChunkSize: 250, + metadataMapChunkSize: 50_000, + wantTargetChunks: 2, + }, + { + name: "100 per chunk", + targetChunkSize: 100, + metadataMapChunkSize: 50_000, + wantTargetChunks: 5, + }, + { + name: "all in one chunk", + targetChunkSize: 1000, + metadataMapChunkSize: 50_000, + wantTargetChunks: 1, + }, + } - for _, resp := range responses[:2] { - _, ok := resp.Item.(*pb.GetTargetGraphResponse_Targets) - assert.True(t, ok, "expected Targets chunk") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + responses, err := ResultToGetTargetGraphResponse(context.Background(), result, tt.targetChunkSize, tt.metadataMapChunkSize) + require.NoError(t, err) + + var targetChunks int + for _, resp := range responses { + if _, ok := resp.Item.(*pb.GetTargetGraphResponse_Targets); ok { + targetChunks++ + } + } + assert.Equal(t, tt.wantTargetChunks, targetChunks) + }) } - _, ok := responses[2].Item.(*pb.GetTargetGraphResponse_Metadata) - assert.True(t, ok, "last response should be Metadata") } diff --git a/example/cmd/query-bench/main.go b/example/cmd/query-bench/main.go index b3504605..28e4e52b 100644 --- a/example/cmd/query-bench/main.go +++ b/example/cmd/query-bench/main.go @@ -112,7 +112,10 @@ func run() error { totalDuration += elapsed fmt.Printf("run %d: targethasher: %v (%d targets)\n", i+1, elapsed.Round(time.Millisecond), len(targethasherResult.TargetNames)) start = time.Now() - response, err := common.ResultToGetTargetGraphResponse(ctx, targethasherResult, common.DefaultTargetChunkSize, common.DefaultMetadataMapChunkSize) // also print the time to convert to GetTargetGraphResponse format + response, err := common.ResultToGetTargetGraphResponse( + ctx, targethasherResult, + common.DefaultTargetChunkSize, common.DefaultMetadataMapChunkSize, + ) if err != nil { return fmt.Errorf("run %d: converting to GetTargetGraphResponse: %w", i+1, err) } From bfeb3019a41eecb464380b3296047cafcdb8ab53 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 10 Jul 2026 13:04:21 -0700 Subject: [PATCH 3/6] Update --- core/common/utils.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/core/common/utils.go b/core/common/utils.go index e4487582..42bac923 100644 --- a/core/common/utils.go +++ b/core/common/utils.go @@ -244,6 +244,10 @@ func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Res } func chunkTargets(targets []*tangopb.OptimizedTarget, chunkSize int) []*tangopb.GetTargetGraphResponse { + if chunkSize <= 0 { + chunkSize = DefaultTargetChunkSize + } + // at least one chunk numChunks := max(1, (len(targets)+chunkSize-1)/chunkSize) @@ -291,6 +295,10 @@ func ChunkMetadata( attrStrValIDToVal map[int32]string, chunkSize int, ) []*tangopb.Metadata { + if chunkSize <= 0 { + chunkSize = DefaultMetadataMapChunkSize + } + targetChunks := splitMap(targetIDToName, chunkSize) attrValChunks := splitMap(attrStrValIDToVal, chunkSize) From c417693a458da850682d22ac5530819c326051ae Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Fri, 10 Jul 2026 13:59:35 -0700 Subject: [PATCH 4/6] Update --- core/common/utils.go | 1 + 1 file changed, 1 insertion(+) diff --git a/core/common/utils.go b/core/common/utils.go index 42bac923..4821608c 100644 --- a/core/common/utils.go +++ b/core/common/utils.go @@ -140,6 +140,7 @@ const cancelCheckInterval = 4096 // ResultToGetTargetGraphResponse converts a Result to a GetTargetGraphResponse. // targetChunkSize controls how many OptimizedTarget entries per stream message. // metadataMapChunkSize controls how many entries per metadata map chunk. +// TODO: move this function to internal/mapper func ResultToGetTargetGraphResponse(ctx context.Context, result targethasher.Result, targetChunkSize, metadataMapChunkSize int) ([]*tangopb.GetTargetGraphResponse, error) { // Map target names to ids. This list is topologically sorted, so the ids are stable. // IDs start at 1 — 0 is reserved as the proto3 "unset" sentinel so consumers using From 4ab07beea842dfd337780311a336e71589da5fd3 Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Sun, 12 Jul 2026 18:02:05 -0700 Subject: [PATCH 5/6] Address review comments: reuse remote var, test metadata chunking Use the already-derived remote local variable instead of re-reading param.Req.BuildDescription.Remote, and exercise metadataMapChunkSize with small values in TestResultToGetTargetGraphResponse_Chunking so metadata chunking is actually verified, not just target chunking. --- core/common/utils_test.go | 16 ++++++++++++---- orchestrator/native_orchestrator.go | 6 +++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/core/common/utils_test.go b/core/common/utils_test.go index df9d5b7e..c399a203 100644 --- a/core/common/utils_test.go +++ b/core/common/utils_test.go @@ -202,24 +202,28 @@ func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { targetChunkSize int metadataMapChunkSize int wantTargetChunks int + wantMetadataChunks int }{ { name: "250 per chunk", targetChunkSize: 250, - metadataMapChunkSize: 50_000, + metadataMapChunkSize: 200, wantTargetChunks: 2, + wantMetadataChunks: 3, }, { name: "100 per chunk", targetChunkSize: 100, - metadataMapChunkSize: 50_000, + metadataMapChunkSize: 100, wantTargetChunks: 5, + wantMetadataChunks: 5, }, { name: "all in one chunk", targetChunkSize: 1000, metadataMapChunkSize: 50_000, wantTargetChunks: 1, + wantMetadataChunks: 1, }, } @@ -229,13 +233,17 @@ func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { responses, err := ResultToGetTargetGraphResponse(context.Background(), result, tt.targetChunkSize, tt.metadataMapChunkSize) require.NoError(t, err) - var targetChunks int + var targetChunks, metadataChunks int for _, resp := range responses { - if _, ok := resp.Item.(*pb.GetTargetGraphResponse_Targets); ok { + switch resp.Item.(type) { + case *pb.GetTargetGraphResponse_Targets: targetChunks++ + case *pb.GetTargetGraphResponse_Metadata: + metadataChunks++ } } assert.Equal(t, tt.wantTargetChunks, targetChunks) + assert.Equal(t, tt.wantMetadataChunks, metadataChunks) }) } } diff --git a/orchestrator/native_orchestrator.go b/orchestrator/native_orchestrator.go index e84241ef..794cda19 100644 --- a/orchestrator/native_orchestrator.go +++ b/orchestrator/native_orchestrator.go @@ -135,9 +135,9 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget } } }() - err = ws.Checkout(ctx, param.Req.BuildDescription.Remote, param.Req.BuildDescription.BaseSha) + err = ws.Checkout(ctx, remote, param.Req.BuildDescription.BaseSha) if err != nil { - return nil, fmt.Errorf("checkout %s@%s: %w", param.Req.BuildDescription.Remote, param.Req.BuildDescription.BaseSha, err) + return nil, fmt.Errorf("checkout %s@%s: %w", remote, param.Req.BuildDescription.BaseSha, err) } logger.Infow("GetTargetGraph: Checked out base revision") @@ -166,7 +166,7 @@ func (b *nativeOrchestrator) GetTargetGraph(ctx context.Context, param GetTarget if err != nil { return nil, fmt.Errorf("compute treehash: %w", err) } - treehashPath := common.GetGraphByTreeHash(param.Req.BuildDescription.Remote, treehash, param.Req.BuildDescription.GetStrategy(), param.Req.GetRequestOptions()) + treehashPath := common.GetGraphByTreeHash(remote, treehash, param.Req.BuildDescription.GetStrategy(), param.Req.GetRequestOptions()) if !param.BypassCache { graphReader, err := storage.NewGraphReader(ctx, b.storage, treehashPath) if err == nil { From 28d54a507180a6564065865dc29714824e6aa63d Mon Sep 17 00:00:00 2001 From: Yushan Lin Date: Sun, 12 Jul 2026 18:18:11 -0700 Subject: [PATCH 6/6] Shrink chunking test data sizes TestChunkTargets and TestResultToGetTargetGraphResponse_Chunking used inflated target counts (250/500) with no bearing on what's being verified. Scale down to 25/50 targets while preserving the same chunk-count expectations. --- core/common/utils_test.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/core/common/utils_test.go b/core/common/utils_test.go index c399a203..3ac8846a 100644 --- a/core/common/utils_test.go +++ b/core/common/utils_test.go @@ -161,13 +161,13 @@ func TestGetComparedTargetsCachePath(t *testing.T) { func TestChunkTargets(t *testing.T) { t.Parallel() - // Create 250 targets, chunk by 100 → expect 3 chunks (100, 100, 50) - targets := make([]*pb.OptimizedTarget, 250) + // Create 25 targets, chunk by 10 → expect 3 chunks (10, 10, 5) + targets := make([]*pb.OptimizedTarget, 25) for i := range targets { targets[i] = &pb.OptimizedTarget{Id: int32(i)} } - responses := chunkTargets(targets, 100) + responses := chunkTargets(targets, 10) require.Len(t, responses, 3) @@ -180,13 +180,13 @@ func TestChunkTargets(t *testing.T) { total++ } } - assert.Equal(t, 250, total) + assert.Equal(t, 25, total) } func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { t.Parallel() - numTargets := 500 + numTargets := 50 result := targethasher.Result{ TargetNames: make([]string, numTargets), Targets: make(map[string]*targethasher.Target, numTargets), @@ -205,23 +205,23 @@ func TestResultToGetTargetGraphResponse_Chunking(t *testing.T) { wantMetadataChunks int }{ { - name: "250 per chunk", - targetChunkSize: 250, - metadataMapChunkSize: 200, + name: "25 per chunk", + targetChunkSize: 25, + metadataMapChunkSize: 20, wantTargetChunks: 2, wantMetadataChunks: 3, }, { - name: "100 per chunk", - targetChunkSize: 100, - metadataMapChunkSize: 100, + name: "10 per chunk", + targetChunkSize: 10, + metadataMapChunkSize: 10, wantTargetChunks: 5, wantMetadataChunks: 5, }, { name: "all in one chunk", - targetChunkSize: 1000, - metadataMapChunkSize: 50_000, + targetChunkSize: 100, + metadataMapChunkSize: 5_000, wantTargetChunks: 1, wantMetadataChunks: 1, },