perf(oneroster): use delta reads with bounded resumable Classroom reconciliation - #80
perf(oneroster): use delta reads with bounded resumable Classroom reconciliation#80Sykezzz wants to merge 3 commits into
Conversation
(cherry picked from commit af01c2a4c8fe75274e861d158e5a4fe4d44ac20f)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08adb8297f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| last_error_code TEXT NOT NULL DEFAULT '', | ||
| version INTEGER NOT NULL DEFAULT 1, | ||
| updated_at REAL NOT NULL DEFAULT 0, | ||
| PRIMARY KEY(domain, alias) |
There was a problem hiding this comment.
Treat the registry key as case-insensitive here, matching the planner and connector's pervasive casefold() semantics. If a SIS later changes only the casing of a section ID (for example, Section_ABC to Section_abc), record_managed_course_desired() inserts a second row rather than updating the first; subsequent verification can then fail on the unique course-ID index or return an arbitrary state after both rows collapse to the same case-folded dictionary key. Use a NOCASE key/unique constraint and handle existing duplicates during migration.
Useful? React with 👍 / 👎.
| rows = conn.execute( | ||
| """ | ||
| SELECT * FROM managed_course_state | ||
| WHERE domain = ? ORDER BY alias COLLATE NOCASE | ||
| """, | ||
| (self.domain,), | ||
| ).fetchall() |
There was a problem hiding this comment.
When aliases is supplied, this query still loads every managed-course row for the domain and filters only after materialization. The planner invokes this method once per 200-course metadata chunk and once per 100-course roster chunk, so a 50,000-course district can deserialize tens of millions of rows during one initial reconciliation, substantially undermining the bounded-read performance this change introduces. Apply the requested alias set in SQL, using bounded batches or a temporary table where necessary.
Useful? React with 👍 / 👎.
1 participant
Summary
Section_<ID>managed courses.Safety
Local evidence
96 passed, 0 failed, 0 skipped in 58.52s24 passed, 2 skipped in 2.11sgam-compat.git diff --checkpassed.Required merge gates
gam-compatexecutes the two locally skipped compatibility checks.