Skip to content

Commit 5b18e1b

Browse files
committed
feat(conflict): file-overlap analyzer using changeset resolver
Add submitqueue/extension/conflict/fileoverlap, a conflict.Analyzer that flags two batches as conflicting when they change a common file. It is the first analyzer to use the capability the extension contract unblocks: it takes only batch identity and resolves each batch's changed files itself through an injected changeset.Resolver, derived from each change's provider details. A shared file is the concrete notion of target overlap, so it reports the existing conflict.ConflictTypeTargetOverlap — the type the contract named but for which no implementation could be written against an identity-only batch. No change to the conflict.Analyzer interface. The example wires a file-overlap-queue to it.
1 parent 215562a commit 5b18e1b

6 files changed

Lines changed: 274 additions & 0 deletions

File tree

example/submitqueue/orchestrator/server/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ go_library(
3030
"//submitqueue/extension/conflict",
3131
"//submitqueue/extension/conflict/all",
3232
"//submitqueue/extension/conflict/fake",
33+
"//submitqueue/extension/conflict/fileoverlap",
3334
"//submitqueue/extension/conflict/none",
3435
"//submitqueue/extension/mergechecker",
3536
"//submitqueue/extension/mergechecker/fake",

example/submitqueue/orchestrator/server/main.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ import (
4949
"github.com/uber/submitqueue/submitqueue/extension/conflict"
5050
"github.com/uber/submitqueue/submitqueue/extension/conflict/all"
5151
conflictfake "github.com/uber/submitqueue/submitqueue/extension/conflict/fake"
52+
"github.com/uber/submitqueue/submitqueue/extension/conflict/fileoverlap"
5253
"github.com/uber/submitqueue/submitqueue/extension/conflict/none"
5354
"github.com/uber/submitqueue/submitqueue/extension/mergechecker"
5455
mcfake "github.com/uber/submitqueue/submitqueue/extension/mergechecker/fake"
@@ -875,12 +876,18 @@ func newQueueRegistry(logger *zap.Logger, scope tally.Scope, resolver changeset.
875876
conflictErrQueue := base
876877
conflictErrQueue.analyzer = conflictfake.New(all.New(), conflictfake.FailAlways)
877878

879+
// file-overlap-queue: a real analyzer that serializes only batches sharing
880+
// a changed file, resolving each batch's files itself via the resolver.
881+
fileOverlapQueue := base
882+
fileOverlapQueue.analyzer = fileoverlap.New(resolver)
883+
878884
return queueRegistry{
879885
def: base,
880886
byQueue: map[string]queueExtensions{
881887
"test-queue": testQueue,
882888
"e2e-test-queue": e2eQueue,
883889
"e2e-conflict-error-queue": conflictErrQueue,
890+
"file-overlap-queue": fileOverlapQueue,
884891
},
885892
}, nil
886893
}
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "fileoverlap",
5+
srcs = ["fileoverlap.go"],
6+
importpath = "github.com/uber/submitqueue/submitqueue/extension/conflict/fileoverlap",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//submitqueue/core/changeset",
10+
"//submitqueue/entity",
11+
"//submitqueue/extension/conflict",
12+
],
13+
)
14+
15+
go_test(
16+
name = "fileoverlap_test",
17+
srcs = ["fileoverlap_test.go"],
18+
embed = [":fileoverlap"],
19+
deps = [
20+
"//submitqueue/core/changeset/fake",
21+
"//submitqueue/entity",
22+
"//submitqueue/extension/conflict",
23+
"@com_github_stretchr_testify//assert",
24+
"@com_github_stretchr_testify//require",
25+
],
26+
)
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# fileoverlap
2+
3+
`fileoverlap` is a `conflict.Analyzer` that reports a conflict between two batches when they change one or more of the same files.
4+
5+
It is the first analyzer to exercise the capability the [extension contract](../../../../doc/rfc/submitqueue/extension-contract.md) unblocks: it is handed only batch identity (the candidate batch and the in-flight batches) and resolves each batch's changed files itself through an injected `changeset.Resolver`, rather than depending on a controller to pre-compute them. This is why the `conflict.Analyzer` contract takes identity and resolves internally — a file-overlap analyzer could not be written against a controller-resolved, identity-only batch.
6+
7+
## Behavior
8+
9+
The files a batch changes are drawn from each change's provider-supplied details. The candidate batch conflicts with an in-flight batch when their changed-file sets intersect; each such in-flight batch is reported once, preserving the in-flight order. A shared file is the concrete notion of *target overlap*, so conflicts are classified as `ConflictTypeTargetOverlap`. A batch that changes no files conflicts with nothing, and an empty in-flight list yields no conflicts. A failure to resolve a batch's changes is returned as a (retryable) error.
10+
11+
File-path intersection is a deliberately simple notion of overlap. A richer one (build targets, ownership boundaries) would be a separate analyzer rather than a change to this one.
Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package fileoverlap provides a conflict.Analyzer that reports a conflict
16+
// between two batches when they change one or more of the same files. It is the
17+
// first analyzer to use the capability the extension contract unblocks: it takes
18+
// only batch identity and resolves each batch's changed files itself through an
19+
// injected changeset resolver, rather than depending on the controller to
20+
// pre-compute them. A shared file is the concrete notion of target overlap, so
21+
// it reports conflict.ConflictTypeTargetOverlap.
22+
package fileoverlap
23+
24+
import (
25+
"context"
26+
"fmt"
27+
28+
"github.com/uber/submitqueue/submitqueue/core/changeset"
29+
"github.com/uber/submitqueue/submitqueue/entity"
30+
"github.com/uber/submitqueue/submitqueue/extension/conflict"
31+
)
32+
33+
// analyzer reports a conflict between batches that change a common file. The
34+
// files a batch changes are resolved from each batch's change details.
35+
type analyzer struct {
36+
resolver changeset.Resolver
37+
}
38+
39+
// New returns a conflict.Analyzer that flags an in-flight batch as conflicting
40+
// when it changes a file the candidate batch also changes. The resolver
41+
// resolves each batch's changed files.
42+
func New(resolver changeset.Resolver) conflict.Analyzer {
43+
return analyzer{resolver: resolver}
44+
}
45+
46+
// Analyze returns one ConflictTypeTargetOverlap Conflict per in-flight batch
47+
// that shares a changed file with batch, preserving the in-flight order. A batch
48+
// that changes no files conflicts with nothing.
49+
func (a analyzer) Analyze(ctx context.Context, batch entity.Batch, inFlight []entity.Batch) ([]conflict.Conflict, error) {
50+
if len(inFlight) == 0 {
51+
return nil, nil
52+
}
53+
54+
candidate, err := a.files(ctx, batch)
55+
if err != nil {
56+
return nil, fmt.Errorf("failed to resolve files for batch %s: %w", batch.ID, err)
57+
}
58+
if len(candidate) == 0 {
59+
return nil, nil
60+
}
61+
62+
var conflicts []conflict.Conflict
63+
for _, other := range inFlight {
64+
files, err := a.files(ctx, other)
65+
if err != nil {
66+
return nil, fmt.Errorf("failed to resolve files for batch %s: %w", other.ID, err)
67+
}
68+
if intersects(candidate, files) {
69+
conflicts = append(conflicts, conflict.Conflict{
70+
BatchID: other.ID,
71+
Type: conflict.ConflictTypeTargetOverlap,
72+
})
73+
}
74+
}
75+
return conflicts, nil
76+
}
77+
78+
// files resolves the set of file paths the batch changes.
79+
func (a analyzer) files(ctx context.Context, batch entity.Batch) (map[string]struct{}, error) {
80+
changes, err := a.resolver.DetailedForBatch(ctx, batch)
81+
if err != nil {
82+
return nil, err
83+
}
84+
files := make(map[string]struct{})
85+
for _, change := range changes.Changes {
86+
for _, file := range change.Details.ChangedFiles {
87+
files[file.Path] = struct{}{}
88+
}
89+
}
90+
return files, nil
91+
}
92+
93+
// intersects reports whether the two sets share any element.
94+
func intersects(a, b map[string]struct{}) bool {
95+
// Iterate the smaller set for fewer lookups.
96+
if len(b) < len(a) {
97+
a, b = b, a
98+
}
99+
for k := range a {
100+
if _, ok := b[k]; ok {
101+
return true
102+
}
103+
}
104+
return false
105+
}
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package fileoverlap
16+
17+
import (
18+
"context"
19+
"errors"
20+
"testing"
21+
22+
"github.com/stretchr/testify/assert"
23+
"github.com/stretchr/testify/require"
24+
25+
changesetfake "github.com/uber/submitqueue/submitqueue/core/changeset/fake"
26+
"github.com/uber/submitqueue/submitqueue/entity"
27+
"github.com/uber/submitqueue/submitqueue/extension/conflict"
28+
)
29+
30+
// detailed builds a BatchChanges whose single change touches the given files.
31+
func detailed(batchID string, files ...string) entity.BatchChanges {
32+
changed := make([]entity.ChangedFile, 0, len(files))
33+
for _, f := range files {
34+
changed = append(changed, entity.ChangedFile{Path: f})
35+
}
36+
return entity.BatchChanges{
37+
BatchID: batchID,
38+
Changes: []entity.ChangeInfo{{Details: entity.ChangeDetails{ChangedFiles: changed}}},
39+
}
40+
}
41+
42+
func TestAnalyze(t *testing.T) {
43+
tests := []struct {
44+
name string
45+
candidate entity.BatchChanges
46+
inFlight map[string]entity.BatchChanges
47+
inFlightIDs []string
48+
wantBatches []string
49+
}{
50+
{
51+
name: "overlap on a shared file conflicts",
52+
candidate: detailed("cand", "a.go", "b.go"),
53+
inFlight: map[string]entity.BatchChanges{
54+
"x": detailed("x", "b.go", "c.go"),
55+
},
56+
inFlightIDs: []string{"x"},
57+
wantBatches: []string{"x"},
58+
},
59+
{
60+
name: "disjoint files do not conflict",
61+
candidate: detailed("cand", "a.go"),
62+
inFlight: map[string]entity.BatchChanges{
63+
"x": detailed("x", "z.go"),
64+
},
65+
inFlightIDs: []string{"x"},
66+
wantBatches: nil,
67+
},
68+
{
69+
name: "only overlapping in-flight batches are reported, in order",
70+
candidate: detailed("cand", "a.go"),
71+
inFlight: map[string]entity.BatchChanges{
72+
"x": detailed("x", "a.go"),
73+
"y": detailed("y", "q.go"),
74+
"z": detailed("z", "a.go"),
75+
},
76+
inFlightIDs: []string{"x", "y", "z"},
77+
wantBatches: []string{"x", "z"},
78+
},
79+
{
80+
name: "candidate with no targets conflicts with nothing",
81+
candidate: detailed("cand"),
82+
inFlight: map[string]entity.BatchChanges{
83+
"x": detailed("x", "a.go"),
84+
},
85+
inFlightIDs: []string{"x"},
86+
wantBatches: nil,
87+
},
88+
}
89+
90+
for _, tt := range tests {
91+
t.Run(tt.name, func(t *testing.T) {
92+
resolver := changesetfake.New().SetDetailed("cand", tt.candidate)
93+
inFlight := make([]entity.Batch, 0, len(tt.inFlightIDs))
94+
for _, id := range tt.inFlightIDs {
95+
resolver.SetDetailed(id, tt.inFlight[id])
96+
inFlight = append(inFlight, entity.Batch{ID: id})
97+
}
98+
99+
got, err := New(resolver).Analyze(context.Background(), entity.Batch{ID: "cand"}, inFlight)
100+
require.NoError(t, err)
101+
102+
var ids []string
103+
for _, c := range got {
104+
assert.Equal(t, conflict.ConflictTypeTargetOverlap, c.Type)
105+
ids = append(ids, c.BatchID)
106+
}
107+
assert.Equal(t, tt.wantBatches, ids)
108+
})
109+
}
110+
}
111+
112+
func TestAnalyze_EmptyInFlight(t *testing.T) {
113+
got, err := New(changesetfake.New()).Analyze(context.Background(), entity.Batch{ID: "cand"}, nil)
114+
require.NoError(t, err)
115+
assert.Empty(t, got)
116+
}
117+
118+
func TestAnalyze_ResolverError(t *testing.T) {
119+
sentinel := errors.New("resolve failed")
120+
resolver := changesetfake.New().FailWith(sentinel)
121+
122+
_, err := New(resolver).Analyze(context.Background(), entity.Batch{ID: "cand"}, []entity.Batch{{ID: "x"}})
123+
require.ErrorIs(t, err, sentinel)
124+
}

0 commit comments

Comments
 (0)