Repository navigation
Conversation
The "Collect Pull Requests" detail collector re-collected every pull
request matching `state != 'closed' OR merged = false`. `merged` stays
false forever on a pull request that was closed without being merged, so
those were re-collected on every incremental run, one request each, for as
long as the repo lived.
The condition was widened to `OR merged = false` so that `merged` gets
filled in after a full sync: `repos/{owner}/{repo}/pulls` does not return
`merged`, `commits`, `additions`, `deletions`, `review_comments` or
`merged_by`, so a pull request that was only ever seen in a list page is
stored without them. That repair is still needed, but it has to end once
the details are in.
Select on `commits = 0` instead. Every pull request has at least one
commit, so it marks exactly the records whose details were never
collected, and it stops matching as soon as they are. Open pull requests
are re-collected as before.
The clauses move into `UnfinishedPullRequestsClauses` so the regression
test runs the same query as the collector.
Closes apache#9240
Signed-off-by: Sadat Nazarli <118483002+dvecy@users.noreply.github.com>
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
1 participant
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
Collect Pull Requestshas a second phase that re-fetches pull requests one by one to refreshthe ones that are not finished yet. It picked them with:
A pull request that was closed without being merged never becomes
merged, so it keeps matchingfor ever: one
GET /pulls/{number}for it on every single run, for as long as the repo exists.#9240 reports ~15k of those on one instance, the same payload stored again and again, and a run
finally dying on a
422from GitHub.Why that condition was there
/pulls(the list endpoint) does not returnmerged,commits,additions,deletions,review_commentsormerged_by. So after a full sync every closed pull request is storedwithout them, and
OR merged = false(44f2db2) was added to fetch each one once more and fillthem in. That repair is still needed — it just never stopped.
Fix
Select on
commits = 0instead ofmerged = false. Every pull request has at least one commit,so
commits = 0means "details were never collected", and it stops being true as soon as theyare. Exactly one case changes: a closed, never merged pull request whose details are already in
the database is no longer re-fetched. Open pull requests, merged ones, and the repair pass after
a full sync behave as before. No schema change, no migration, no config change.
The query moved into
UnfinishedPullRequestsClausesso the test runs the collector's own queryinstead of a copy of it.
One thing to be aware of
A closed pull request that is reopened later is no longer picked up by an incremental run. That
is already the case for merged ones,
github_graphqldoes the same (it only refreshesstate = 'OPEN'), and a full sync reconciles it. Catching reopens incrementally would need thelist phase to page by
sort=updated— happy to file a follow-up issue if you want that tracked.Tests
New
TestGithubUnfinishedPullRequestsseeds one pull request per case and asserts the exact setthe query returns. On
mainit fails with[1 2 4 5 6]instead of[1 2 4 5]; with the fix itpasses.
Locally against PostgreSQL 16:
go test ./plugins/github/...all packages ok,go vetclean,golangci-lint run ./plugins/github/...0 issues,gofmt -l plugins/github/empty. Thecondition is plain portable SQL, so MySQL and PostgreSQL behave the same.
Does this close any open issues?
Closes #9240