Skip to content

Commit af0ded2

Browse files
committed
feat(demo): open independent pull requests in parallel
## Summary ### Why? Opening a pull request is several round trips to the provider — cut a branch, commit each file, open the request — and the run did them one after another. For a large `-count` that was most of the run's wall time, spent waiting on the network rather than on the queue. Worse for a demo whose whole subject is contention: a request the queue has not been given yet cannot contend with anything. Serial creation delayed the overlap the tool exists to show, so the early part of every run was the least interesting part of it. Nothing about independent changes required the wait. Each branches from the same base and writes files no other change touches — the sharded paths guarantee that — so the ordering was an artifact of the loop. ### What? Independent pull requests are now created concurrently, bounded by `-concurrency` (default 5, `CONCURRENCY` on `make demo-pr`). Each is still enqueued the moment it exists, so the queue starts working sooner as well as being fed faster. The limit is deliberate rather than arbitrary. The provider is a shared service with its own opinion about burst rates, and the point of the tool is to feed the queue, not to discover how fast a repository can be hammered. Lower it if a provider starts refusing bursts. `createAndEnqueue` splits into the two shapes it always had, which were tangled together in one loop: - **independent** runs through a bounded group, collecting into an indexed slice so the run's own order survives workers finishing in whatever order the provider answers them. - **stacked** stays strictly sequential, and cannot be otherwise: each change is based on the branch of the one before it and must see its content, so the next branch cannot be cut until the previous head exists. It ignores `-concurrency` rather than pretending to honour it. The shared state the workers touch — the tracker's rows and the table — was already mutex-guarded, because the status poll has always run concurrently with creation. The GitHub client holds only immutable fields and builds a fresh request per call. ## Test Plan ✅ `bazel test //service/submitqueue/demo/pr:go_default_test` — configuration validation now covers the new flag: zero and negative concurrency are rejected, one is accepted as plain sequential rather than treated as invalid, and the run shape reports the limit only when it is above one. ✅ `bazel test //submitqueue/client:go_default_test --features=race` — clean under the race detector, including `TestTrackerConcurrentPollAndUpdate`, which drives concurrent updates against a running poll. That is the shared state these workers now contend for, and the reason no new locking was needed. ✅ `bazel test //...` — 103 packages pass. Not run against a live provider: this needs a real repository and token, and Docker image builds are failing in this environment. What a live run would add is the provider's own reaction to five concurrent creators — burst limits and secondary rate limits — which is exactly what the flag exists to turn down, and what no local test can tell us.
1 parent d097a1b commit af0ded2

5 files changed

Lines changed: 238 additions & 83 deletions

File tree

Makefile

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ export SQ_PROVIDER_CONFIG_DIR ?= $(REPO_ROOT)/service/submitqueue/demo/provider/
5656
DEMO_REPO ?= behinddwalls/sq-demo
5757
COUNT ?= 3
5858
FILES ?= 3
59+
CONCURRENCY ?= 5
5960
STACKED ?= false
6061
SINCE ?= 1h
6162
LIMIT ?= 50
@@ -155,11 +156,12 @@ clean-proto: ## Clean generated proto files
155156
@rm -f $(foreach p,$(PROTO_PACKAGES),$(p)/protopb/*.pb.go $(p)/protopb/*.pb.yarpc.go)
156157
@echo "Proto clean complete!"
157158

158-
demo-pr: ## Create N PRs in the demo repo, enqueue each as it is created, and watch (COUNT=3 FILES=3; needs GITHUB_TOKEN)
159+
demo-pr: ## Create N PRs in the demo repo, enqueue each as it is created, and watch (COUNT=3 FILES=3 CONCURRENCY=5; needs GITHUB_TOKEN)
159160
@$(BAZEL) run //service/submitqueue/demo/pr -- \
160161
-repo $(DEMO_REPO) \
161162
-count $(COUNT) \
162163
-files $(FILES) \
164+
-concurrency $(CONCURRENCY) \
163165
-stacked=$(STACKED) \
164166
-addr $(GATEWAY_ADDR) \
165167
-queue $(QUEUE) \

doc/howto/PROVIDER-E2E.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,15 @@ Opening pull requests by hand gets old fast. `demo-pr` creates them, enqueues th
9797
make demo-pr # 3 independent PRs, each enqueued as it is created
9898
make demo-pr COUNT=8 # more traffic
9999
make demo-pr FILES=8 # wider changes, more files per PR
100+
make demo-pr CONCURRENCY=1 # create them one at a time
100101
make demo-pr STACKED=true # one stack, enqueued as a single request
101102
make demo-pr LAND=false # create only, print the land command
102103
```
103104

104105
Each pull request is enqueued the moment it exists, so the queue is already working on the first while the last is still being opened. That overlap is the point: a queue holding one request at a time never batches, never analyzes a conflict against another batch, and never speculates. Nothing is awaited until every request is in.
105106

107+
Independent pull requests are created **five at a time** by default (`CONCURRENCY`). Opening one is several round trips — a branch, a commit per file, the pull request itself — so creating them serially was most of what a large run spent its time on, and it delayed the overlap the demo exists to show. A stack ignores the setting: each of its changes is based on the branch before it, so the next cannot be cut until the previous head exists. Lower it if the provider starts refusing bursts.
108+
106109
The table is there from the start — one row per land request, drawn before the first pull request exists and filled in as the run proceeds. Whatever is happening right now is a single line underneath it, so creating and enqueuing does not scroll the table away:
107110

108111
```

service/submitqueue/demo/pr/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ go_library(
1212
"//api/base/mergestrategy/protopb:go_default_library",
1313
"//platform/base/change/github:go_default_library",
1414
"//submitqueue/client:go_default_library",
15+
"@org_golang_x_sync//errgroup:go_default_library",
1516
],
1617
)
1718

service/submitqueue/demo/pr/main.go

Lines changed: 191 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@
2323
// batch, and never speculates; those behaviors only appear when requests
2424
// overlap. The table watches all of them at once.
2525
//
26+
// Independent pull requests are opened several at a time (-concurrency), since
27+
// each is several round trips to the provider and nothing about them depends on
28+
// the others. A stack cannot be: every change in it is based on the branch
29+
// before it, so the next cannot be cut until the previous head exists.
30+
//
2631
// Two shapes of change, because the pipeline treats them differently:
2732
//
2833
// - independent (default): each pull request targets the base branch and is
@@ -56,6 +61,7 @@ import (
5661
mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb"
5762
githubchange "github.com/uber/submitqueue/platform/base/change/github"
5863
"github.com/uber/submitqueue/submitqueue/client"
64+
"golang.org/x/sync/errgroup"
5965
)
6066

6167
func main() {
@@ -68,22 +74,23 @@ func main() {
6874

6975
// config is everything the run needs, resolved from flags and the environment.
7076
type config struct {
71-
repo string
72-
base string
73-
count int
74-
files int
75-
stacked bool
76-
prefix string
77-
land bool
78-
watch bool
79-
addr string
80-
tls bool
81-
tokenEnv string
82-
queue string
83-
strategy string
84-
token string
85-
apiRoot string
86-
host string
77+
repo string
78+
base string
79+
count int
80+
files int
81+
concurrency int
82+
stacked bool
83+
prefix string
84+
land bool
85+
watch bool
86+
addr string
87+
tls bool
88+
tokenEnv string
89+
queue string
90+
strategy string
91+
token string
92+
apiRoot string
93+
host string
8794
}
8895

8996
func parseFlags() config {
@@ -92,6 +99,8 @@ func parseFlags() config {
9299
flag.StringVar(&c.base, "base", "main", "branch the changes target")
93100
flag.IntVar(&c.count, "count", 3, "how many pull requests to create")
94101
flag.IntVar(&c.files, "files", 3, "fewest files each pull request touches; the actual count varies a little above it")
102+
flag.IntVar(&c.concurrency, "concurrency", 5,
103+
"how many pull requests to create at once; a stack ignores it, being sequential by nature")
95104
flag.BoolVar(&c.stacked, "stacked", false, "chain the pull requests and enqueue them as one stack")
96105
flag.StringVar(&c.prefix, "prefix", "demo", "branch name prefix")
97106
flag.BoolVar(&c.land, "land", true, "enqueue each pull request as it is created")
@@ -110,14 +119,11 @@ func parseFlags() config {
110119
}
111120

112121
func run(ctx context.Context, cfg config) error {
113-
if cfg.token == "" {
114-
return fmt.Errorf("GITHUB_TOKEN is not set; it is the same credential the stack uses")
115-
}
116-
if cfg.count < 1 {
117-
return fmt.Errorf("-count must be at least 1")
122+
if err := cfg.validate(); err != nil {
123+
return err
118124
}
119-
owner, repo, ok := strings.Cut(cfg.repo, "/")
120-
if !ok || owner == "" || repo == "" {
125+
owner, repo, _ := strings.Cut(cfg.repo, "/")
126+
if owner == "" || repo == "" {
121127
return fmt.Errorf("-repo %q must be owner/name", cfg.repo)
122128
}
123129
strategy, err := client.ParseStrategy(cfg.strategy)
@@ -199,14 +205,40 @@ func shape(cfg config) string {
199205
if cfg.stacked {
200206
return "stacked, enqueued as one request once the chain exists"
201207
}
208+
if cfg.concurrency > 1 {
209+
return fmt.Sprintf("independent, %d at a time, each enqueued as soon as it is created", cfg.concurrency)
210+
}
202211
return "independent, each enqueued as soon as it is created"
203212
}
204213

214+
// validate rejects a configuration the run cannot proceed with.
215+
func (c config) validate() error {
216+
if c.token == "" {
217+
return fmt.Errorf("GITHUB_TOKEN is not set; it is the same credential the stack uses")
218+
}
219+
if c.count < 1 {
220+
return fmt.Errorf("-count must be at least 1")
221+
}
222+
if c.concurrency < 1 {
223+
return fmt.Errorf("-concurrency must be at least 1")
224+
}
225+
if c.files < 1 {
226+
return fmt.Errorf("-files must be at least 1")
227+
}
228+
if _, _, ok := strings.Cut(c.repo, "/"); !ok {
229+
return fmt.Errorf("-repo %q must be owner/name", c.repo)
230+
}
231+
return nil
232+
}
233+
205234
// change is one pull request this run created.
206235
type change struct {
207236
number int
208237
url string
209238
branch string
239+
// headSHA is the commit the pull request now points at, which the next
240+
// change in a stack branches from.
241+
headSHA string
210242
// uri is the SubmitQueue change URI pinning the pull request to its head.
211243
uri string
212244
}
@@ -289,80 +321,102 @@ func createAndEnqueue(
289321
tag, baseSHA string,
290322
t *client.Tracker,
291323
) ([]change, error) {
292-
created := make([]change, 0, cfg.count)
324+
if cfg.stacked {
325+
return createStack(ctx, gh, sq, cfg, strategy, tag, baseSHA, t)
326+
}
327+
return createIndependent(ctx, gh, sq, cfg, strategy, tag, baseSHA, t)
328+
}
329+
330+
// createIndependent opens the pull requests concurrently, up to the configured
331+
// limit, enqueuing each the moment it exists.
332+
//
333+
// Independent changes have nothing to say to each other: each branches from the
334+
// same base and writes files no other change touches, so the only reason to
335+
// create them one at a time was that the loop did. Creating a pull request is
336+
// several round trips to the provider — a branch, a commit per file, the pull
337+
// request itself — and doing that serially is most of what a large run spends
338+
// its time on. It also delays the overlap the demo exists to show, since the
339+
// queue cannot work on requests that have not been submitted yet.
340+
//
341+
// The limit is there because the provider is a shared service with its own
342+
// opinion about burst rates, and because the point is to feed the queue, not to
343+
// find out how fast a repository can be hammered.
344+
func createIndependent(
345+
ctx context.Context,
346+
gh *githubClient,
347+
sq *client.Client,
348+
cfg config,
349+
strategy mergestrategypb.Strategy,
350+
tag, baseSHA string,
351+
t *client.Tracker,
352+
) ([]change, error) {
293353
rows := t.Rows()
354+
// Indexed rather than appended: the workers finish in whatever order the
355+
// provider answers them, and the caller still wants the run's own order.
356+
created := make([]change, cfg.count)
294357

295-
parentBranch, parentSHA := cfg.base, baseSHA
296-
for i := 1; i <= cfg.count; i++ {
297-
// A stack is one request, so every change lands on the single row.
298-
target := rows[0]
299-
if !cfg.stacked {
300-
target = rows[i-1]
301-
}
358+
group, groupCtx := errgroup.WithContext(ctx)
359+
group.SetLimit(cfg.concurrency)
302360

303-
branch := fmt.Sprintf("%s/%s/%d", cfg.prefix, tag, i)
304-
t.Note("creating branch %s", branch)
305-
if err := gh.createBranch(ctx, branch, parentSHA); err != nil {
306-
return nil, fmt.Errorf("create branch %s: %w", branch, err)
307-
}
361+
for i := 1; i <= cfg.count; i++ {
362+
group.Go(func() error {
363+
c, err := createOne(groupCtx, gh, cfg, tag, baseSHA, cfg.base, i, t, rows[i-1])
364+
if err != nil {
365+
return err
366+
}
367+
created[i-1] = c
308368

309-
// Each file is its own commit, so the pull request arrives as a range of
310-
// commits rather than a single edit. The last one is the head the change
311-
// URI pins.
312-
var headSHA string
313-
fileCount := changeFileCount(tag, i, cfg.files)
314-
for k := 1; k <= fileCount; k++ {
315-
path := changeFilePath(tag, i, k)
316-
body := fmt.Sprintf("change %d of run %s\nfile %d of %d\n", i, tag, k, fileCount)
317-
t.Note("committing %s (%d/%d)", path, k, fileCount)
318-
319-
message := fmt.Sprintf("demo change %d (run %s): file %d of %d", i, tag, k, fileCount)
320-
sha, err := gh.commitFile(ctx, branch, path, body, message)
369+
if !cfg.land {
370+
return nil
371+
}
372+
t.Note("enqueuing #%d", c.number)
373+
sqid, err := sq.Land(groupCtx, cfg.queue, urisOf([]change{c}), strategy)
321374
if err != nil {
322-
return nil, fmt.Errorf("commit %s to %s: %w", path, branch, err)
375+
return err
323376
}
324-
headSHA = sha
325-
}
377+
t.Update(func() { rows[i-1].SQID, rows[i-1].Submitted = sqid, time.Now() })
378+
return nil
379+
})
380+
}
326381

327-
t.Note("opening pull request for %s", branch)
328-
number, url, err := gh.openPR(ctx, fmt.Sprintf("demo change %d (run %s)", i, tag), branch, parentBranch)
329-
if err != nil {
330-
return nil, fmt.Errorf("open pull request for %s: %w", branch, err)
331-
}
382+
if err := group.Wait(); err != nil {
383+
return nil, err
384+
}
385+
return created, nil
386+
}
332387

333-
c := change{
334-
number: number, url: url, branch: branch,
335-
uri: githubchange.ChangeID{
336-
Scheme: "github", Host: cfg.host, Org: gh.owner, Repo: gh.repo,
337-
PRNumber: number, HeadCommitSHA: headSHA,
338-
}.String(),
339-
}
340-
created = append(created, c)
341-
// The cell is what the table shows for this change: the pull request
342-
// number, clickable where the terminal allows it.
343-
cell := client.Cell{Text: fmt.Sprintf("#%d", number), URL: url}
344-
t.Update(func() { target.Cells = append(target.Cells, cell) })
345-
346-
if cfg.stacked {
347-
// The next change builds on this one, so it sees this change's
348-
// content and its pull request is based on this branch.
349-
parentBranch, parentSHA = branch, headSHA
350-
continue
351-
}
352-
if !cfg.land {
353-
continue
354-
}
355-
t.Note("enqueuing #%d", number)
356-
sqid, err := sq.Land(ctx, cfg.queue, urisOf([]change{c}), strategy)
388+
// createStack opens the pull requests one after another, each based on the one
389+
// before it, and submits the whole chain as a single request.
390+
//
391+
// This one cannot be parallelized, and not for want of trying: a change is
392+
// based on the branch of the change before it and must see its content, so the
393+
// next branch cannot be cut until the previous head exists.
394+
func createStack(
395+
ctx context.Context,
396+
gh *githubClient,
397+
sq *client.Client,
398+
cfg config,
399+
strategy mergestrategypb.Strategy,
400+
tag, baseSHA string,
401+
t *client.Tracker,
402+
) ([]change, error) {
403+
rows := t.Rows()
404+
created := make([]change, 0, cfg.count)
405+
406+
parentBranch, parentSHA := cfg.base, baseSHA
407+
for i := 1; i <= cfg.count; i++ {
408+
// A stack is one request, so every change lands on the single row.
409+
c, err := createOne(ctx, gh, cfg, tag, parentSHA, parentBranch, i, t, rows[0])
357410
if err != nil {
358411
return nil, err
359412
}
360-
t.Update(func() { target.SQID, target.Submitted = sqid, time.Now() })
413+
created = append(created, c)
414+
parentBranch, parentSHA = c.branch, c.headSHA
361415
}
362416

363417
// The stack goes in as one request, which is only possible now that every
364418
// change in it exists.
365-
if cfg.stacked && cfg.land {
419+
if cfg.land {
366420
t.Note("enqueuing the stack")
367421
sqid, err := sq.Land(ctx, cfg.queue, urisOf(created), strategy)
368422
if err != nil {
@@ -373,6 +427,61 @@ func createAndEnqueue(
373427
return created, nil
374428
}
375429

430+
// createOne cuts a branch from parentSHA, writes the change's files to it, and
431+
// opens a pull request against parentBranch, recording it on the given row.
432+
func createOne(
433+
ctx context.Context,
434+
gh *githubClient,
435+
cfg config,
436+
tag, parentSHA, parentBranch string,
437+
i int,
438+
t *client.Tracker,
439+
target *client.Row,
440+
) (change, error) {
441+
branch := fmt.Sprintf("%s/%s/%d", cfg.prefix, tag, i)
442+
t.Note("creating branch %s", branch)
443+
if err := gh.createBranch(ctx, branch, parentSHA); err != nil {
444+
return change{}, fmt.Errorf("create branch %s: %w", branch, err)
445+
}
446+
447+
// Each file is its own commit, so the pull request arrives as a range of
448+
// commits rather than a single edit. The last one is the head the change
449+
// URI pins.
450+
var headSHA string
451+
fileCount := changeFileCount(tag, i, cfg.files)
452+
for k := 1; k <= fileCount; k++ {
453+
path := changeFilePath(tag, i, k)
454+
body := fmt.Sprintf("change %d of run %s\nfile %d of %d\n", i, tag, k, fileCount)
455+
t.Note("committing %s (%d/%d)", path, k, fileCount)
456+
457+
message := fmt.Sprintf("demo change %d (run %s): file %d of %d", i, tag, k, fileCount)
458+
sha, err := gh.commitFile(ctx, branch, path, body, message)
459+
if err != nil {
460+
return change{}, fmt.Errorf("commit %s to %s: %w", path, branch, err)
461+
}
462+
headSHA = sha
463+
}
464+
465+
t.Note("opening pull request for %s", branch)
466+
number, url, err := gh.openPR(ctx, fmt.Sprintf("demo change %d (run %s)", i, tag), branch, parentBranch)
467+
if err != nil {
468+
return change{}, fmt.Errorf("open pull request for %s: %w", branch, err)
469+
}
470+
471+
c := change{
472+
number: number, url: url, branch: branch, headSHA: headSHA,
473+
uri: githubchange.ChangeID{
474+
Scheme: "github", Host: cfg.host, Org: gh.owner, Repo: gh.repo,
475+
PRNumber: number, HeadCommitSHA: headSHA,
476+
}.String(),
477+
}
478+
// The cell is what the table shows for this change: the pull request
479+
// number, clickable where the terminal allows it.
480+
cell := client.Cell{Text: fmt.Sprintf("#%d", number), URL: url}
481+
t.Update(func() { target.Cells = append(target.Cells, cell) })
482+
return c, nil
483+
}
484+
376485
// urisOf is the change URIs the run pinned, in caller order.
377486
func urisOf(cs []change) []string {
378487
out := make([]string, 0, len(cs))

0 commit comments

Comments
 (0)