reload RPC server manifest on SIGHUP (#1684) #1699
Conversation
Requires git-only hologram branch TODO: update hologram version + do another release
From the sighup until completion, the sever enters an error state where only builtin commands work
- management of tasks should keep functioning properly
Add new builtin command 'status'
- returns 'pid', 'status' enum, 'timestamp'
- if the status is 'error', an 'error' field is populated
- just a Dict[str, Any] with, only 'message' for now
There was a problem hiding this comment.
This is really slick @beckjake! I gave it a spin locally and have a couple of questions (noted inline) and one piece of feedback: If a compilation error occurs, then the rpc server gets into a state where the non-builtin endpoints aren't loaded. The same thing occurs while the manifest is recompiling: it's ok for dbt to not response to requests while this re-compilation is happening, but the response should probably be different.
A query to /run, for instance, results in:
request={'jsonrpc': '2.0', 'method': 'run', 'id': 1, 'params': {'timeout': None, 'sql': 'c2VsZWN0IDEgYXMgaWQ=', 'name': 'foo'}}
{'error': {'code': -32601, 'message': 'Method not found'}, 'id': 1, 'jsonrpc': '2.0'}
Can we instead surface a more semantically meaningful error indicating that there is an error with the project?
core/dbt/task/rpc_server.py
Outdated
|
|
||
| @contextmanager | ||
| def signhup_replace(): | ||
| """Å context manager. Replace the current sighup handler with SIG_IGN on |
There was a problem hiding this comment.
Because that's what option + A does on a mac, and I guess I hit option while typing this!
|
|
||
| # current_handler should be the handler unless we're already loading a | ||
| # new manifest. So if the current handler is the ignore, there was a | ||
| # double-hup! We should exit and not touch the signal handler, to make |
There was a problem hiding this comment.
How would a double-hup happen? And why would the response be to exit?
There was a problem hiding this comment.
The how is easy: send a lot of sighups to the same process at the same time
For the why... I have no idea, which is why I used SIG_IGN instead of SIG_DFL.
| @@ -2,8 +2,8 @@ | |||
| import multiprocessing | |||
| import os | |||
| import random | |||
| import signal | |||
There was a problem hiding this comment.
these tests are great, but can we move them out of 042_sources_test? I didn't realize these existed!
There was a problem hiding this comment.
Yeah, that's probably been a long time coming - they just rely on the same base project as the sources tests and I was too lazy to duplicate them all.
|
todo:
|
Fixes #1684
statusRPC command to the rpc server, and adds a SIGHUP handler to the rpc server that reloads the manifest. There is some complicated behavior around signals, locks, threads, and processes thanks to the complicated intersection of locks, signals, andThread.join()in python and posix.statusreturns:pid,status,timestamp, and optionallyerror(if the status iserror). Thetimestampfield is the time the last status change occurred.run_projectand then reload the manifest, you'd be able to kill and control the initial process while the manifest was loading and even after it.While I was in here I fixed up some hologram/mypy stuff (mostly pattern-related things and the
deprecationsmodule), so this depends on hologram 0.0.2 now.