search

chrisboulton / php-resque

PHP port of resque (Workers and Queueing)
MIT License
3.43k stars 759 forks source link

Job tracking improvements #210

Open Aeon opened 10 years ago

Aeon commented 10 years ago

all tests still pass :persevere:

danhunsaker commented 10 years ago

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

Aeon commented 10 years ago

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

danhunsaker commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

chrisboulton commented 8 years ago

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

Aeon commented 8 years ago

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

Aeon commented 7 years ago

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

danhunsaker commented 7 years ago

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

:+1: