-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Reinitialize pantsd for most configuration changes #10035
Reinitialize pantsd for most configuration changes #10035
Conversation
…tches how we have always used it in the engine), and break the false dependency from the NailgunServer to the Scheduler: it only needs the Executor. [ci skip-jvm-tests]
…r the server to be long-lived and the services/scheduler to be short lived. [ci skip-rust-tests] [ci skip-jvm-tests]
Commits are useful to look at independently. In particular, the last commit could use special attention, as it adjusts which bootstrap options are marked |
…ch can drop and recreate them based on options changes. [ci skip-jvm-tests]
…e expanding the options that affect Scheduler creation to include the entire set of bootstrap options. Please review this commit closely. [ci skip-jvm-tests]
9698e0d
to
8be37b0
Compare
@@ -188,11 +189,20 @@ def override_thread_logging_destination_to_just_stderr(self): | |||
def match_path_globs(self, path_globs: PathGlobs, paths: Iterable[str]) -> bool: | |||
return cast(bool, self.lib.match_path_globs(path_globs, tuple(paths))) | |||
|
|||
def nailgun_server_await_bound(self, scheduler, nailgun_server) -> int: | |||
return cast(int, self.lib.nailgun_server_await_bound(scheduler, nailgun_server)) | |||
def nailgun_server_await_bound(self, nailgun_server) -> int: |
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.
Can you put a type anno on nailgun_server
?
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.
I can't... the type is unnameable right now because it's self.lib.PyNailgunServer
rather than something importable.
I think that we can fix that (I'm not sure that we need to use importlib
anymore?), and I thought about fixing it here, but it was off topic.
# Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
Problem
Currently we restart pantsd for most configuration changes, and exclude a small set of bootstrap options (by marking them
daemon=False
) that should not trigger a restart. But in most cases, restarting is heavy-handed. We'd like to be able to keep more and more of our state alive over time as we continue to remove global mutable state (in order to allow us to tackle #7654, among other things).Additionally, the pantsd client currently implements the fingerprinting that decides when the server should restart, which blocks moving the pantsd client to rust. We'd like the client to only need to interact with a small set of options to simplify its implementation.
Solution
Move the nailgun server out of the
PantsService
model and directly into thePantsDaemon
class. Share aPantsDaemonCore
between the daemon andDaemonPantsRunner
that encapsulates the currentScheduler
and all live services. Finally, have thePantsDaemonCore
implement fingerprinting to decide when to reinitialize/recreate theScheduler
(without restarting) and trim down the options that trigger a restart (daemon=True
) to only those that are used to start the daemon itself (rather than to create theScheduler
).Result
pantsd
will stay up through the vast majority of options changes (with the exception of a handful of "micro-bootstrap" options), and will instead reinitialize theScheduler
for bootstrap options changes with some useful output when it does so.Example:
This prepares to port the client to rust, and unblocks a fix for #8200, by having the
PantsDaemon
class tear down the nailgun server cleanly in the foreground if any services exit. Fixes #6114, fixes #7573, and fixes #10041.