Skip to content
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

Requeue and remove #703

Merged
merged 3 commits into from Sep 27, 2012
Merged

Requeue and remove #703

merged 3 commits into from Sep 27, 2012

Conversation

KensoDev
Copy link

A problem I always had with the default resque requeue process is that it actually has a basic flaw.

This flaw is that the requeue process does not remove the items from the failed queue.

This means, that if you have items in your failed queue and you requeue them, the next time each item will get queued up twice.

What I actually expect to happen (and implemented here), is that when I requeue a task, it will get queued up and removed from the fail, this way there will be no duplicates.

I do realize this can create some confusion, so let me know what you think

@steveklabnik
Copy link
Member

I like this basic idea, but we have no tests. Any thoughts on that?

@KensoDev
Copy link
Author

@steveklabnik it's calling already tested methods.
requeue and remove are tested and the new method just calls those two.

@steveklabnik
Copy link
Member

Sure, but we're changing behavior, but we don't have any tests that break when it's changed, that's what I'm worried about.

@steveklabnik
Copy link
Member

Also, this no longer merges cleanly.

Anyway, I think we're all 👍 on this change, but a CHANGELOG entry would be nice, since this does change behavior.

@trevorturk
Copy link
Contributor

I'd like to see the button renamed, too. Maybe from "Retry" to "Requeue" or something? Since we're changing behavior, it'd be nice to give a heads up.

@KensoDev
Copy link
Author

@steveklabnik gotcha. will add a changelog entry.
@trevorturk will also rename the button.

"Requeue" makes sense to me.

@KensoDev
Copy link
Author

@steveklabnik changed the button text, is it just me or there's no CHANGELOG to document those kind of changes?
I can add a CHANGELOG file in a separate pull and add this if you'd like.

@trevorturk
Copy link
Contributor

It's HISTORY.md

On Thu, Sep 27, 2012 at 12:53 PM, Avi Tzurel notifications@github.com wrote:

@steveklabnik changed the button text, is it just me or there's no CHANGELOG to document those kind of changes?
I can add a CHANGELOG file in a separate pull and add this if you'd like.


Reply to this email directly or view it on GitHub.

@KensoDev
Copy link
Author

I thought about History.md but I saw it's very version dependent and there's no indication what is the next version.

@trevorturk
Copy link
Contributor

Start a new section? Like "unreleased" or something. I just think this is a good one to add, and we might as well get started.

@steveklabnik
Copy link
Member

Yeah, add a 2.0 section 'unreleased' at the top.

@KensoDev
Copy link
Author

Added the history entry.

@steveklabnik
Copy link
Member

Thanks. Still can't be merged, can you rebase? Soon as that happens, I click the button, we all go home happy. :)

@KensoDev
Copy link
Author

Sure. Rebasing...

@KensoDev
Copy link
Author

Rebased.

steveklabnik added a commit that referenced this pull request Sep 27, 2012
@steveklabnik steveklabnik merged commit b92ae76 into resque:master Sep 27, 2012
@steveklabnik
Copy link
Member

❤️ ❤️ ❤️

@trevorturk
Copy link
Contributor

👍 one of my biggest peeves with resque-web == fixed! 😁

@KensoDev
Copy link
Author

My pleasure.

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.

None yet

3 participants