Add debug message on job fail - #101
Conversation
|
Pretty sure i already said on IRC that this is not the right solution. The |
|
The job id is missing from the log message also. And debug is probably not the right level. |
|
Not to mention the lack of tests. |
b052cb9 to
5bf532e
Compare
|
Ok, moved the message from Minion/Job.pm to Minion/Command/worker/minion.pm. Sounds like a much more appropriate place for it. Still no tests though, mostly because I have no idea how to test it :) Wether it should be debug or some other level can be discussed, but the other messages in that file are debug, and for me, I mostly use such things for debugging. If you really want to do anything with those error messages, I guess you should handle them in other ways in the app itself. |
|
That's a lot of code for what could have been a one-liner. |
|
Removed one of the functions, _dequeue, and fixed the indentation and some spacing to match the other code. Still, a bit more than a one-liner, but that wouldn't be very readable code :) |
| $worker->on(dequeue => sub { pop->once(spawn => \&_spawn) }); | ||
| $worker->on(dequeue => sub { | ||
| $_[1]->once(failed => \&_failed); | ||
| $_[1]->once(spawn => \&_spawn); |
There was a problem hiding this comment.
Don't use $_[1] over multiple lines.
| my($job, $error) = @_; | ||
| my($id, $task) = ($job->id, $job->task); | ||
| $error =~ s/\n//; | ||
| $job->app->log->debug(qq{Job "$id", task "$task" failed: $error}); |
There was a problem hiding this comment.
The message format is rather inconsistent.
There was a problem hiding this comment.
Feel free to provide positive feedback on what a a consistent message should look like instead....
2 participants
Summary
Just a simple patch to add a debug log on job fail.
Could possibly add some check if there already are on-fail events hooks, but not sure how to do that :)
Motivation
I find it really useful to actually see if a job fail in the terminal running ./app minion worker, and not have to inspect the web interface or add promises or hooks to jobs.
References
Was discussed on #mojo earlier this week