Skip to content

Refactor: Invert dependency between query history and remote quries managers#1396

Merged
charisk merged 3 commits intomainfrom
charisk/remote-query-history-inversion
Jun 23, 2022
Merged

Refactor: Invert dependency between query history and remote quries managers#1396
charisk merged 3 commits intomainfrom
charisk/remote-query-history-inversion

Conversation

@charisk
Copy link
Copy Markdown
Contributor

@charisk charisk commented Jun 21, 2022

Currently, the RemoteQueriesManager depends on the QueryHistoryManager and related models e.g. QueryHistoryInfo. This is a bit problematic because the RemoteQueriesManager ends up having knowledge about query history stuff and can receive events that are potentially for local queries.

This PR inverts that dependency and makes RemoteQueriesManager a dependency of QueryHistoryManager. The RemoteQueriesManager is completely independent of query history and simply publishes events that the QueryHistoryManager can consume. I think this way makes more sense because the query history area has to be aware of local/remote queries, but remote queries don't have to necessarily be aware of the query history.

Checklist

N/A

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@charisk charisk requested review from a team as code owners June 21, 2022 14:36
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible.

constructor(
private readonly qs: QueryServerClient,
private readonly dbm: DatabaseManager,
private readonly localQueriesInterfaceManager: InterfaceManager,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rename InterfaceManager to something like LocalQueriesInterfaceManager.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'd like to do that but I thought I'd leave it for a separate PR.

There are a lot of things that refer to local queries that aren't prefixed with local (since local was the only queries you could run until 6 months ago!). It'd be good to gradually make the terminology more specific.

Comment thread extensions/ql-vscode/src/query-history.ts Outdated
Comment thread extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts Outdated
Comment thread extensions/ql-vscode/src/query-history.ts Outdated
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tidying this up! 🧹

I tried locally, and I wonder if something has changed with reporting progress of the Run Variant Analysis command? 🔄

Specifically, the final stage of the command ("Sending request") now stays on-screen the whole time until the results are back. I'd expect it to disappear as soon as we've successfully scheduled the run:

image

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Jun 22, 2022

Thanks for tidying this up! 🧹

I tried locally, and I wonder if something has changed with reporting progress of the Run Variant Analysis command? 🔄

Specifically, the final stage of the command ("Sending request") now stays on-screen the whole time until the results are back. I'd expect it to disappear as soon as we've successfully scheduled the run:

image

Great catch! Not sure how I missed this.

I'm not sure what could be causing this. I'll take a look tomorrow. @aeisenberg please let me know if you have any ideas. I don't think I've changed the "run remote query" code path so I'm a bit puzzled as to why this would be happening.

@charisk
Copy link
Copy Markdown
Contributor Author

charisk commented Jun 22, 2022

Bug fixed now - there was an await on the execution of the monitor command and things just happened to work before my changes.

Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense 👍

@charisk charisk merged commit fd4b602 into main Jun 23, 2022
@charisk charisk deleted the charisk/remote-query-history-inversion branch June 23, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants