Skip to content

Conversation

@ashishtilara
Copy link

  • if the "Perform()" function returns result, store it along with process status
  • return process status+result in Resque_Job_Status->get()
  • allow user to delete the status from redis

Prefix while enqueuing

  • to add one more level of saperation, (e.g. If you have a class between application and resque, the class can handle this, so a prefix can be userid or something in background, so one user do not access another user's queue status items)

- if the "Perform()" function returns result, store it along with process status
- return process status+result in Resque_Job_Status->get()
- allow user to delete the status from redis

Prefix while enqueuing
- to add one more level of saperation, (e.g. If you have a class between application and resque, the class can handle this, so a prefix can be userid or something in background, so one user do not access another user's queue status items)
@Rockstar04
Copy link
Contributor

Would you be able to look at why the test are failing?

@danhunsaker
Copy link
Contributor

This is two separate changes, and that is generally frowned on with GitHub pull requests, or indeed pull requests of any kind. If the upstream maintainer (@chrisboulton, in this case) wants to merge in one change but not the other (say, the return-value-with-status feature, but not the job-status-prefix feature), he either has to merge in both and then revert the one he doesn't want, or ask you to go back and remove those changes yourself before accepting the merge. So I'd definitely recommend splitting these changes into two separate branches on your fork, then submitting pull requests for each separately.

That said, both of these changes break compatibility with the Ruby Resque web interface, which has (historically, at least) been a design feature of PHP-Resque (mostly so we don't have to clone that as well). Granted, the return-value-with-status feature probably won't break it a whole lot, but the job-status-prefix feature will break it in a big way. This effect might be lessened if the prefix were on the other side of job (as in 'job:' . $prefix . ':' . $id . ':status'), but it's still a major change to the in-Redis data structure. So there's a chance this PR would be rejected even if it was split into separate requests appropriately.

Still, your best bet is to go back and make these changes in separate branches, submit separate PRs, and see what happens. There's a good chance your changes can be tweaked (in accordance with discussion surrounding each) and still be accepted upstream.

Last bit of advice: make sure your indentation is consistent with the rest of the project. That's one of the most common reasons a PR is delayed, for this project at least. Best of luck!

@danhunsaker
Copy link
Contributor

@Rockstar04 Looking at the results in Travis, that's because the tests weren't updated properly.

@ashishtilara
Copy link
Author

Thank you @danhunsaker, I will work on the items you suggested and send pull request again.

Really appreciate your comments 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants