-
Notifications
You must be signed in to change notification settings - Fork 645
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
[3.x] Ability to see if a job is delayed #755
Conversation
This looks okay but I would use a different color since blue is often associated with something that goes okay. Maybe the same yellow from the pause symbol? Also I'd just say "delayed" and not "delayed job". Maybe add that timestamp on hover? |
This reverts commit 78f83ea.
That looks better imo but let's see what others say :) |
@Cannonb4ll first, very nice!! I like this PR!
|
Just committed the tooltip on hover: For this I had to move popper.js import above the bootstrap import because the docs say so and jQuery import to the app.js in order for the tooltips to work. Then I made a directive for the tooltip so each new record would also get the tooltip instance bound to its element. (Otherwise new records that show up dynamically will not have the tooltip anymore) This also resulted that I removed the jQuery imports from the specific components. I also took the liberty to reduce JS size by importing the specific lodash package "take" in "Stacktrace.vue" as an example. We could do this with more components as I noticed the full lodash library is imported there which gives a lot of overhead of JS. Let me know what you think! |
It's weird to me to show this label for a job that is already finished processing. It only makes sense to me to see it while pending and even better if it shows a human readable thing like "Delayed for 10 minutes" |
@taylorotwell i think you right. It is better to show the label when the delay is not passed. But where do you want to show the "Delayed for 10 minutes"? Only in the hover? I think it is better to say "xx minutes" in the hover. |
I agree with you @taylorotwell, which option do you prefer more? Option 1: Option 2: |
If you choose option 1 than remove the "Delayed for" in the hover. The label are say that it is a delayed job. |
I'm fine with the tooltip... I think purple is a bit "in your face" for this informational stuff... I would have gone with a gray background or something. |
Is this ready for review on your end? |
Yes, it is. Not sure why the travis builds fail randomly though. |
@@ -38,6 +38,8 @@ public function index(Request $request) | |||
$jobs = $this->jobs->getRecent($request->query('starting_at', -1))->map(function ($job) { | |||
$job->payload = json_decode($job->payload); | |||
|
|||
$job->delayed = strtotime(optional(unserialize($job->payload->data->command))->delay); |
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.
If you are unserializing the entire command to get this won't that re-issue the database queries needed to get the Eloquent models / collections required by the job?
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.
@taylorotwell Just tried, indeed it will introduce queries to be run whenever a model is type hinted into the job class, this will introduce reduced performance once there are jobs with a lot of models.
Is there a way to disable this magic once we call an unserialise on the payload?
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.
Not that I can think of off of the top of my head. It may be possible to extract the delay from the raw serialized string but I'm not sure.
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 am going to think and try some stuff out. Will get back on this, thanks, good find 👍
Ok, requesting a review again for this one. What I did is remove the PHP way with unserialising as this was causing DB queries. Instead I used the phpunserialize JS package which was already included. In order to do this I exported each TR row to its own component to make proper use of computed records. Now there are no DB queries anymore. |
Thank you for this! |
Sometimes I find myself in a situation where I want to see if a job was dispatched with a delay, this PR allows you to see if it is via a badge.
Examples: