Job tracking improvements by Aeon · Pull Request #210 · chrisboulton/php-resque

Aeon · 2014-09-06T05:09:48Z

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

all tests still pass 😣

Aeon · 2014-09-06T05:18:06Z

lib/Resque/Redis.php

should we add the rest of them?

HDEL HEXISTS HGETALL HINCRBY HINCRBYFLOAT HKEYS HLEN HMGET HSCAN HSETNX HVALS

danhunsaker · 2014-09-06T11:44:53Z

This completely breaks interoperability with other Resque implementations, and thus will be rejected.

Aeon · 2014-09-10T23:15:17Z

@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!

Aeon · 2014-09-10T23:16:26Z

lib/Resque/Job/Status.php

we were discarding started on update previously.

danhunsaker · 2014-09-18T20:35:44Z

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.

Aeon · 2014-09-19T23:17:12Z

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 :)

danhunsaker · 2014-09-20T02:10:11Z

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.

Aeon · 2014-09-20T02:26:27Z

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?

danhunsaker · 2014-09-20T02:42:39Z

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.

wedy · 2014-09-20T10:36:08Z

@Aeon, try amend and push --force it again, it will trigger travis again

chrisboulton · 2016-11-21T21:50:26Z

@danhunsaker, @Aeon - what was the conclusion on this change breaking interop? I had a quick look through, and nothing immediately stands out?

Aeon · 2016-11-21T23:42:07Z

@chrisboulton I rolled back the interop breaking change, so this should be entirely safe at this point

On Nov 21, 2016, at 1:50 PM, Chris Boulton notifications@github.com wrote:

@danhunsaker, @Aeon - what was the conclusion on this change breaking interop? I had a quick look through, and nothing immediately stands out?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

sergeyklay · 2016-11-22T00:18:29Z

lib/Resque/Job/Status.php

+	/**
+	 * generate job id consistently
+	 */
+	private static function generateId($id) {


Could you please use common style? I mean put opening braces for methods to the next line

Using private visibility means we can't just inherit the class because public methods call private methods. How about protected visibility?

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.

sergeyklay · 2016-11-22T00:20:20Z

lib/Resque/Job/Status.php

+	 *
+	 * @return mixed False if the status is not being monitored, otherwise the status packet array or the individual field
+	 */
+	private function fetch($field = false)


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 · 2017-02-24T09:16:46Z

Let me know if there's any other concerns with this pull request :)

danhunsaker · 2017-02-24T09:48:05Z

@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.

👍

Closes chrisboulton/php-resque#210

Aeon reviewed Sep 6, 2014
View reviewed changes

Aeon force-pushed the job-tracking-improvements branch from d93a3fb to c1f91d6 Compare September 10, 2014 23:13

Aeon reviewed Sep 10, 2014
View reviewed changes

lib/Resque/Job/Status.php

Copy link

Author

Aeon Sep 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were discarding started on update previously.

Aeon force-pushed the job-tracking-improvements branch from c1f91d6 to 1033d33 Compare September 10, 2014 23:25

Aeon force-pushed the job-tracking-improvements branch from 1033d33 to b22a5e3 Compare November 8, 2016 02:03

Aeon mentioned this pull request Nov 8, 2016

Maybe this project needs new maintainer ? #274

Open

sergeyklay suggested changes Nov 22, 2016

View reviewed changes

enhancements to job status tracking

16218a1

- 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 November 29, 2016 02:02

danhunsaker added a commit to resque/php-resque that referenced this pull request Dec 11, 2018

Apply old PR 210

dec0d9c

Closes chrisboulton/php-resque#210

Comments

Conversation

Aeon commented Sep 6, 2014

Uh oh!

Aeon Sep 6, 2014

Choose a reason for hiding this comment

Uh oh!

danhunsaker commented Sep 6, 2014

Uh oh!

Aeon commented Sep 10, 2014

Uh oh!

Aeon Sep 10, 2014

Choose a reason for hiding this comment

Uh oh!

danhunsaker commented Sep 18, 2014

Uh oh!

Aeon commented Sep 19, 2014

Uh oh!

danhunsaker commented Sep 20, 2014

Uh oh!

Aeon commented Sep 20, 2014

Uh oh!

danhunsaker commented Sep 20, 2014

Uh oh!

wedy commented Sep 20, 2014

Uh oh!

chrisboulton commented Nov 21, 2016

Uh oh!

Aeon commented Nov 21, 2016

Uh oh!

sergeyklay Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

sergeyklay Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aeon Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

sergeyklay Nov 22, 2016

Choose a reason for hiding this comment

Uh oh!

Aeon Nov 29, 2016

Choose a reason for hiding this comment

Uh oh!

Aeon commented Feb 24, 2017

Uh oh!

danhunsaker commented Feb 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sergeyklay Nov 22, 2016 •

edited

Loading