Skip to content
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

Ensure databases are re-registered when query server restarts #734

Merged
merged 2 commits into from
Jan 29, 2021

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Jan 29, 2021

This commit fixes #733. It does it by ensuring that the query server
emits an event when it restarts the query server. The database manager
listens for this even and properly re-registers its databases.

A few caveats though:

  1. Convert query restarts to using a command that includes progress.
    This will ensure that errors on restart are logged properly.
  2. Because we want to log errors, we cannot use the vscode standard
    EventEmitters. They run in the next tick and therefore any errors
    will not be associated with this command execution.
  3. Update the default cli version to run integration tests against to
    2.4.2.
  4. Add a new integration test that fails if databases are not
    re-registered.

Checklist

  • 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.
  • [n/a] @github/docs-content-dsp has been cc'd in all issues for UI or other user-facing changes made by this pull request.

Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Appreciate the quick fix and the test! A few minor suggestions.

extensions/ql-vscode/src/queryserver-client.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/databases.ts Outdated Show resolved Hide resolved
extensions/ql-vscode/src/databases.ts Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@aeisenberg aeisenberg force-pushed the aeisenberg/qs-restart branch 5 times, most recently from a14a093 to 4d9f2a0 Compare January 29, 2021 17:55
This commit fixes github#733. It does it by ensuring that the query server
emits an event when it restarts the query server. The database manager
listens for this even and properly re-registers its databases.

A few caveats though:

1. Convert query restarts to using a command that includes progress.
   This will ensure that errors on restart are logged properly.
2. Because we want to log errors, we cannot use the vscode standard
   EventEmitters. They run in the next tick and therefore any errors
   will not be associated with this command execution.
3. Update the default cli version to run integration tests against to
   2.4.2.
4. Add a new integration test that fails if databases are not
   re-registered.
@aeisenberg
Copy link
Contributor Author

aeisenberg commented Jan 29, 2021

The integration tests were failing because of a real bug. Fixed now. I'm glad we upgraded the tests to run on 2.4.2

@adityasharad
Copy link
Contributor

Looks good. Do you want a change note for the upgrade bugfix?

@aeisenberg
Copy link
Contributor Author

Looks good. Do you want a change note for the upgrade bugfix?

Sure...why not...

@aeisenberg aeisenberg merged commit f154206 into github:main Jan 29, 2021
@aeisenberg aeisenberg deleted the aeisenberg/qs-restart branch January 29, 2021 19:24
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.

"No result from server" after changing codeql path
2 participants