Add timeout and process failure event handlers to tasks #1698
Conversation
|
By analyzing the blame information on this pull request, we identified @Tarrasch, @themalkolm and @daveFNbuck to be potential reviewers |
| # kill and wait for it to be killed. | ||
| os.kill(process.pid, signal.SIGUSR2) | ||
| while process.is_alive(): | ||
| time.sleep(.1) |
There was a problem hiding this comment.
I'll admit this feels like a weird thing to do in a unit test... but I tried os.waitpid and some other mocking approaches and I couldn't get either to work.
There was a problem hiding this comment.
Couldn't you just create a mock TaskProcess that returns False for is_alive and has a nonzero exit code?
|
Does @ulzha (event guru ^^) want to review? |
casey-green
force-pushed
the
task_timeout_process_killed_handlers
branch
2 times, most recently
from
21f03db
to
c99b4e7
Compare
|
Addressed comments, thanks! |
Current coverage is 53.48%@@ master #1698 diff @@
==========================================
Files 94 59 -35
Lines 10285 8890 -1395
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 8104 4754 -3350
- Misses 2181 4136 +1955
Partials 0 0
|
|
ping @Tarrasch - following up on this. Anything to do on my side? |
|
LGTM. Though I wish somebody else too could have reviewed it too ... Anyways, good job! :) |
* spotify/master: (25 commits) Version 2.2.0 Add tests for hashing parameters (spotify#1719) Update call to iteritems in luigi/tools/deps: deprecated in Python 3 (spotify#1749) Reset terminal colors in external_program (spotify#1742) Caches get_autoconfig_client on a per-thread basis Fix bug with GCSFlagTarget Add additional event handlers to tasks (spotify#1698) Reduce number of get_params calls in common_params. Removes redundant function definitions from rpc and server (spotify#1734) Fix salesforce default content type (spotify#1724) Rename MockTarget class variable _fn to path Remove MockTarget path property Deprecated LocalTarget fn propery Add note about underscore in parameter names (spotify#1729) Remove tracking url callback hack (spotify#1722) Consistent Luigi spelling in docs (spotify#1723) Update example_top_artists.rst (spotify#1662) Add combiner to docstrings in mrrunner Add luigi-deps-tree visualising tool (spotify#1680) Adding release step for Debian packages. (spotify#1718) ...
Add additional event handlers to tasks
This adds two new event handlers:
Motivation and Context
We use event handlers to profile and monitor tasks (i.e. trigger alerts when tasks fail). These two task failure types will fail silently in our system because they don't trigger the
FAILURE(or any other) event type.Tests
I included unit tests which test each event handler.