dvecy opened a new pull request, #9250:
URL: https://github.com/apache/devlake/pull/9250
### Summary
The GitHub convert subtasks filter their input with
```go
if stateManager.IsIncremental() {
since := stateManager.GetSince()
if since != nil {
clauses = append(clauses, dal.Where("github_updated_at >= ?", since))
}
}
```
`GetSince()` is `_devlake_subtask_state.prev_started_at` — the wall-clock
time the previous run
of that subtask started. `github_updated_at` is when GitHub last changed the
record. Comparing
them means a tool record that DevLake wrote or corrected *after* the
previous run is skipped
whenever GitHub has not touched it since then — on that run and on every run
after it.
That is #9150: the tool layer is right, the domain record is missing or
still `OPEN` with no
close/merge dates, and a later successful collection never repairs it.
The clearest case, and the one in the report, is a pull request closed years
ago whose `merged`
flag a later collection filled in: `github_updated_at` is 2019, `since` is a
few hours ago, so
the corrected row never reaches `pull_requests`.
### Fix
Filter on the tool record's own `updated_at`. Gorm maintains it on every
write, so it means
"DevLake last wrote this row", which is what the watermark is for. Verified:
an upsert that
flips `state` and `merged` advances `updated_at` and leaves
`github_updated_at` untouched.
This is not a new convention — it is the one already in use:
- every GitLab, Jira and Taiga convert subtask (`dal.Where("updated_at >= ?
", since)`)
- the GitHub repo, release, account and cicd-job convert subtasks
- the worked example in the `StatefulDataConverter` doc comment
(`backend/helpers/pluginhelper/api/data_convertor_stateful.go`), which uses
`_tool_jira_issues.updated_at >= ?`
Changed here are the five GitHub convert subtasks whose watermark is on
their own table:
pull requests, issues, pull request comments, issue comments and runs.
Deliberately not changed: `pr_label`, `pr_issue`, `pr_commit`, `pr_review`,
`review`,
`issue_label` and `issue_assignee` gate on a joined *parent* table's
`github_updated_at`.
Switching those raises a separate question — whether the parent or the child
table should carry
the watermark — and I did not want to decide it inside a bug fix. Happy to
open a follow-up
issue, or to extend this PR if you prefer it all in one place.
### Scope and limits, stated explicitly
- Existing domain records that were already skipped are **not** healed by
this change; their
`updated_at` is in the past. One full sync or re-transform reconciles
them. This answers the
reporter's question about a supported way to reconcile.
- No schema change, no migration.
- `updated_at` is not indexed on these tables while `github_updated_at` is.
The queries are
already driven by the `repo_id` / `connection_id` predicate, so the plan
is unchanged in
practice; adding an index would be a separate migration if you want one.
- This is a different cause from the `BatchSave.lastErr` theory discussed on
the issue, and it
does not depend on any write having failed.
### Tests
New `TestGithubPrConvertorIncremental`
(`backend/plugins/github/e2e/pr_convertor_incremental_test.go`) seeds three
tool records and a
`_devlake_subtask_state` row, then runs the real `Convert Pull Requests`
subtask in incremental
mode:
| pull request | `github_updated_at` | `updated_at` | expected |
| --- | --- | --- | --- |
| 29 | 2019-08-08 | after the previous run | converted, `MERGED` |
| 30 | after the previous run | after the previous run | converted |
| 31 | before the previous run | before the previous run | skipped |
On `main` it fails with `expected: []int{29, 30}` / `actual: []int{30}` —
pull request 29 never
reaches the domain layer. With the fix it passes, and 31 is still correctly
skipped, so the
watermark keeps doing its job.
`DataFlowTester.Subtask` cannot drive this: it flushes
`_devlake_subtask_state` and forces
`FullSync` on every call. The test runs the subtask through a context that
preserves the state,
the same approach as `plugins/youtrack/e2e/stateful_scenario_test.go`.
Locally, against PostgreSQL 16:
```
go test -count=1 ./plugins/github/... # all packages ok
go vet ./plugins/github/... # clean
golangci-lint run ./plugins/github/... # 0 issues
gofmt -l plugins/github/ # no output
```
All plugin e2e suites were also run one at a time against a fresh database,
as
`scripts/e2e-test-go-plugins.sh` does. `dora/e2e`, `zentao/e2e` and
`schema_e2e` fail, and they
fail identically on an unmodified checkout of `main` on this machine, so
they are unrelated to
this change.
### Does this close any open issues?
Closes #9150
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
