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

Failures 2.0 failure objects #1115

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@cade
Copy link
Contributor

cade commented Sep 11, 2013

This is a big change, so I'm open to suggestions/additions/improvements.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 11, 2013

Coverage Status

Changes Unknown when pulling 5da7449 on cade:failures-2.0-failure-objects into * on resque:master*.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Sep 11, 2013

My initial feedback is that this is super awesome, I'm gonna see what other people think too though.

@cade

This comment has been minimized.

Copy link
Contributor Author

cade commented Sep 11, 2013

Also, still getting intermittent failures on CI for the legacy suite hook tests.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Sep 11, 2013

Oh, this is kinda dumb, but I prefer it "sets the foo" do to it "should set the foo" do. The should is superfluous. Been meaning to change the older tests to that style, but we should use this for new tests too.

@steveklabnik

This comment has been minimized.

@cade

This comment has been minimized.

Copy link
Contributor Author

cade commented Sep 11, 2013

@steveklabnik Thanks for the feedback!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 11, 2013

Coverage Status

Changes Unknown when pulling 376b945 on cade:failures-2.0-failure-objects into * on resque:master*.

# @option opts [Integer] :offset - the number of failures to offset the results by (ex. pagination)
# @option opts [Integer] :limit - the maximum number of failures returned (ex. pagination)
# @return [Array<Hash>, Hash{Symbol=>Array<Hash>}]
def self.all(opts = {})

This comment has been minimized.

@tomblomfield

tomblomfield Sep 11, 2013

I'm not sure about the behaviour of this method - it sometimes returns an array, and sometimes a hash.

That makes any method which calls this needs to be unnecessarily complex, eg see the definition of Failure::Base.filter_by_class_name_from

Why not just always return an array - this would keep compatibility with 1.x? If you really need the hash behaviour somewhere, have a separate method that explicitly does that.

This comment has been minimized.

@cade

cade Sep 11, 2013

Author Contributor

@tomblomfield Thanks for the feedback!

In my opinion, if we wanted to lock this method down to always return the same value type, it should always return a hash. The original 1.x design was based on a single failure queue (:failed), so returning failures in an array made sense. But with the move toward multiple failure queues, pushing failures from different failure queues into a singular array seems counterintuitive to me.

As it's currently written, it obviously tries to take the user's arguments into consideration and just "do the right thing", though I agree, with contextually differing return values, there is possibility for confusion. Also, you have a good point that limiting to a single return value would simplify some of the internals.

@tarcieri / @steveklabnik - Any thoughts on this interface?

This comment has been minimized.

@tomblomfield

tomblomfield Sep 24, 2013

RedisMultiQueue inherits from Base and overloads class method all, changing the method signature.

Choosing to return only a hash feels like an unnecessary gotcha because it's different from both previous versions of Resque and other single-queue implementations - ie your Redis class has the same method, but it returns an array. It would be nice if Redis and RedisMultiQueue had the same basic API, as far as possible.

You could extend RedisMultiQueue to provide the additional functionality - ie implement an .all_by_queue method.

This comment has been minimized.

@cade

cade Sep 24, 2013

Author Contributor

You're absolutely right that it doesn't comply with previous versions of Resque or the Redis class.

I'm not considering compatibility with the Redis class based on the "Multiple failure queues or GTFO" point from the Failures 2.0 wiki. It's my expectation that the Redis class will likely be deleted, and the thinking behind the failure system will fundamentally shift from 1.x. I've only kept Redis up to date because deleting Redis without further core team/community discussion felt presumptuous on my part.

Thanks for helping to think this through with me. Hopefully the verdict on the fate of Redis will shed some light on how to normalize the return value of RedisMultiQueue::all if the current return situation is deemed to violate POLS.

@JackDanger

This comment has been minimized.

Copy link
Contributor

JackDanger commented Jul 17, 2014

Needs a rebase. I recommend merging this because it is awesome.


# Decodes raw result objects from Redis and optionally yields them to a block
# @api private
def process_results(results, index, &block)

This comment has been minimized.

@JackDanger

JackDanger Jul 17, 2014

Contributor

Note: this &block argument is unused and can be removed.

cade added some commits Sep 5, 2013

Change behavior of ::all for retrieving failure records
The former implementation of ::all was renamed to slice. ::all now
accepts an options hash consisting of :queue, :class_name, :offset or :limit.

This is intended to be an initial implementation of @tarcieri's suggestion on
how to fix the Failure interface from the Failures 2.0 wiki.
Promote failures to domain objects by making Resque::Failure a class
This is to address @tarcieri's "Failures aren't objects" point from the
Failures 2.0 wiki. (Also, the latter half of "More like ActiveRecord".)
Remove legacy-facing Failure#[] method
Also deletes the legacy tests that fail without this backward compatibility
@cade

This comment has been minimized.

Copy link
Contributor Author

cade commented Jul 17, 2014

@JackDanger rebased and removed the block param you pointed out. Thanks for the feedback!

@yaauie

This comment has been minimized.

Copy link
Member

yaauie commented Jul 17, 2014

I'll merge this when the build succeeds; I really appreciate the review from @JackDanger on this and the bump to keep it moving.

@cade & @JackDanger: if you could both check your e-mails, you'll find an invitation to have more freedom to contribute to this project; I appreciate both of your contributions and trust your instincts more than I trust my ability to keep you unblocked.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Oct 29, 2015

Hey there!

Sorry that your PR has (probably) languished. Resque development has been on hold for a long time. That said, @hoffmanc and I (as well as others, hopefully!) are going to be getting back to it. Which brings me to this PR.

Resque 2.0 is basically on hold, forever. We'll be swapping the 1-x-stable and master branches sometime next week. For more details, see here: http://words.steveklabnik.com/rescuing-resque-again

As such, master won't be taking any improvements at this time, as this redux is basically dying off. So, thank you for the pull request, but I'm going to give it a close. Sorry again about the wait. If you'd like to pitch in on the new stuff, we'd love to have you! Just let us know.

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