-
Notifications
You must be signed in to change notification settings - Fork 218
add commands for warming the overlay-base cache #4195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d83409d to
4d6c60c
Compare
9073228 to
daad02a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds functionality to warm the overlay-base cache by introducing a second query server configured with specific cache-warming flags. The implementation adds new commands for cache warming operations and manages database registration between the two query servers.
Key changes:
- Adds a second query server instance dedicated to cache warming with flags
--no-evaluate-as-overlay,--cache-at-frontier, and--warm-cache-only - Implements three new commands for warming overlay-base cache: for single queries, multiple queries, and query suites
- Introduces database registration switching logic to ensure only one query server can register a database at a time
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| local-databases.test.ts | Adds test coverage for database registration switching between query servers |
| query-server-client.ts | Adds cache warming configuration flags and descriptive logging for the warming query server |
| local-query-run.ts | Conditionally skips showing results when warming cache |
| local-queries.ts | Implements cache warming commands and routes them to the appropriate query server |
| ast-cfg-commands.ts | Updates function call to include new warmOverlayBaseCache parameter |
| extension.ts | Initializes the second query server and wires up restart logic |
| database-manager.ts | Implements database registration switching between query servers |
| loggers.ts | Adds dedicated logger for the cache warming query server |
| commands.ts | Defines type signatures for new cache warming commands |
| package.json | Registers new commands in VS Code command palette and context menus |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
daad02a to
f903a5d
Compare
nickrolfe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall approach makes sense, and adding a second query-server was less invasive than I feared, but I have a few suggestions.
Co-authored-by: Nick Rolfe <[email protected]>
Co-authored-by: Nick Rolfe <[email protected]>
Co-authored-by: Nick Rolfe <[email protected]>
b9c1afd to
9947c9d
Compare
9947c9d to
56f8652
Compare
nickrolfe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
This PR adds a second query server that is configured with
and several commands:
These commands are based on
but they run on the second query server and their results are not loaded into the "CodeQL Query Results" view.
One complication of this approach with two query servers is that only one of them can register the database at any given time. This is handled via
withDatabaseInQsForWarmingOverlayBaseCacheand seems to work well in practice. It does mean that running a query while a cache warming is ongoing will result in an error message. That seems better than silently running on the wrong kind of query server, though, which would be the effect on an alternative implementation with a single query server that gets killed and restarted.