Job tracking improvements #210
Conversation
| @@ -45,6 +45,9 @@ class Resque_Redis | |||
| 'setex', | |||
| 'get', | |||
| 'getset', | |||
| 'hset', | |||
| 'hmset', | |||
| 'hget', | |||
| 'setnx', | |||
There was a problem hiding this comment.
should we add the rest of them?
HDEL
HEXISTS
HGETALL
HINCRBY
HINCRBYFLOAT
HKEYS
HLEN
HMGET
HSCAN
HSETNX
HVALS
|
This completely breaks interoperability with other Resque implementations, and thus will be rejected. |
Aeon
force-pushed
the
job-tracking-improvements
branch
from
d93a3fb to
c1f91d6
Compare
|
@danhunsaker ahh... good point, I thought this type of job tracking was php-resque specific. In that case, I have reverted the hash changes, and left only the bugfix and enhancements in the pull request. Please review and let me know if there is any other feedback! Thank you! |
| $statusPacket = array( | ||
| 'status' => $status, | ||
| 'updated' => time(), | ||
| 'started' => $this->fetch('started') |
There was a problem hiding this comment.
we were discarding started on update previously.
Aeon
force-pushed
the
job-tracking-improvements
branch
from
c1f91d6 to
1033d33
Compare
|
Looks good overall from here. We'll have to keep in mind that the sanity check is there in case we, the Ruby implementation, or a plugin decide to add status values to the allowed list, but that's a relatively minor concern, I think. |
|
I'm not sure why it says Travis build failed, can I trigger another build somehow? I am pretty sure it's safe to merge :) |
|
Check what test(s) failed and make sure it's not your PR that breaks it/them. It might just be a bad test that wasn't updated with a different change, or a problem with Travis itself. |
|
No tests are failing for me. As far as I can tell, the travis build timed out trying to install phpunit? https://travis-ci.org/chrisboulton/php-resque/jobs/34966352 I don't think I have access to re-run the build without pushing a new commit? |
|
Yes, but since @chrisboulton is the only one with repo write access, only he can see the rebuild button and trigger the rebuild process. I doubt there would be an issue, though, since every other test config passed, and the failure on the only one that failed was due to a Composer issue (frighteningly common in automated environments), so it's probably safe to merge anyway. |
|
@Aeon, try amend and push --force it again, it will trigger travis again |
Aeon
force-pushed
the
job-tracking-improvements
branch
from
1033d33 to
b22a5e3
Compare
|
@danhunsaker, @Aeon - what was the conclusion on this change breaking interop? I had a quick look through, and nothing immediately stands out? |
|
@chrisboulton I rolled back the interop breaking change, so this should be entirely safe at this point
|
| /** | ||
| * generate job id consistently | ||
| */ | ||
| private static function generateId($id) { |
There was a problem hiding this comment.
Could you please use common style? I mean put opening braces for methods to the next line
There was a problem hiding this comment.
Using private visibility means we can't just inherit the class because public methods call private methods. How about protected visibility?
There was a problem hiding this comment.
Done and done. Though, I see a number of other classes in the project that have functions with inconsistent opening braces, so didn't realize there was a consistent style to follow.
| * | ||
| * @return mixed False if the status is not being monitored, otherwise the status packet array or the individual field | ||
| */ | ||
| private function fetch($field = false) |
There was a problem hiding this comment.
Could you please put protected/private at the end of class?
- add helper methods to support fetching started/updated timestamps - do not discard `started` timestamp on update - simplify job id mapping - sanity check `update()` to accept valid statuses only
Aeon
force-pushed
the
job-tracking-improvements
branch
from
b22a5e3 to
16218a1
Compare
|
Let me know if there's any other concerns with this pull request :) |
|
@chrisboulton - In its first incarnation, it changed the way jobs are stored in Redis, but once that was pointed out, those changes were removed. I looked through the rest and it looks good, now. @Aeon - Nothing I'm aware of that should block this change being merged. Just waiting for Chris to have the time to review and merge. 👍 |
startedtimestamp on updateupdate()to accept valid statuses onlyall tests still pass 😣