-
Notifications
You must be signed in to change notification settings - Fork 131
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
Refactor job scheduler to use memdb for jobs #782
Conversation
9fd093d
to
6cca57f
Compare
9a89195
to
9223958
Compare
5da77d4
to
ed4aa4d
Compare
ed4aa4d
to
7f558b9
Compare
In the old test we would be checking what *all* schemas are available for a given path after indexing via walker. This part of API is however being deprecated in favour of more straightforward one (see state.ProviderSchemaStore -> ProviderSchema()) which picks the single most appropriate schema from a list of candidates and retains the decision logic as an implementation detail within it, so the whole list of candidates is not really available from the outside (by design), hence not something we should test from the outside. ProviderSchemaStore itself already has plenty of tests testing the decision logic within, which is better place for testing this anyway.
7f558b9
to
ff634f4
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.
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.
Awesome work!
🚢 !
This functionality has been released in v0.26.0 of the language server. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
This represents 2nd stage of the refactoring per #719
Depends on #771
Closes #719
Closes #768
Closes #775
Closes #800
Why
As described in #719 the existing logic is hard to test and requires
time.Sleep
in cases where dependent pieces of work are scheduled, i.e. synchronization is difficult/non-existent. This leads to a number of flakey tests, making testing of any other bug or feature more difficult.Lack of synchronization also causes e.g. completion to use outdated data, as reported in #768 or #775 due to
textDocument/completion
running beforetextDocument/didChange
had the chance to finish applying any changes to the document and re-parse anything.New
job
packageThis is to make enqueueing and consuming jobs from various places easier without running into import cycles.
Example
New
jobs
memdb tableA new memdb table was created to store queued/running jobs and to provide synchronization via watch channels in memdb.
New general-purpose
scheduler
(package)Scheduling was previously done via
moduleLoader
, essentially implementation detail ofModuleManager
, making everything depend onModuleManager
.moduleLoader
internally also used priority queue implemented viacontainer/heap
. Queue had to be resorted on every insertion (even though no sorting took place most of the time, unless user opened/closed affected files). De-duplication was done via individualModule
fields indicating the state of the operation, making the whole logic very verbose and error prone.terraform-ls/internal/terraform/module/module_loader.go
Lines 192 to 217 in 526f22c
Instead each pending job now has its own entry in the new
jobs
memdb table, allowing for more efficient querying when checking for duplicate jobs and distinguishing between open and closed directory.Relatedly, a (mostly) general purpose scheduler was created to simply execute arbitrary
Func
s aboveDir
s and possibly execute anyDefer
ed functions (dependent jobs).Removal of module manager
Module manager is no more as the individual responsibilities were split between scheduler + jobs memdb table.