collectiveidea/delayed_job — pull request #452 by TiagoCardoso1983 (2012-11-16 14:35:42+00:00)
Title: Composite keys implementation

Concerning https://github.com/collectiveidea/delayed_job/pull/451 Added some specing to some changes. Couldn't exactly see how could I correctly specced the psych_ext:102 update, since it is enclosed under a huge case statement which code routine seems to be the same implemented in the active_record file. As suggestion I would isolate this routine in some method and call it in both places, this way I could spec the routine in isolation. But, as stated before, goal is here to provide some sort of multiple primary keys support without breaking the main standard case, which is numerical and incremental id.

I've thought about the performance gap discussion and (unless it is possible with currying, which I don't think it is, but one could still investigate) I don't think it is a dealbreaker. In the standard case scenario (one primary key) we are talking about allocating memory for the new array, whereas its values should be the same ( try this: a = {:a => 1} ; a[:a].object_id == a.values_at(:a).first.object_id ). I don't really see this overhead becoming such a nuisance. I even thought about calling conditionally calling the values_at on cases where the id would be more than one, but even then one could have the same discussion about having an additional cpu cycle :) .

As a last request: if for some other reason you decide not to accept this pull request, could you at least follow the above mentioned suggestion and isolate the ActiveRecord find calls into one function? Because if delayedjob doesn't support composite primary keys, I would still like to have the option to cleanly patch this functionality. If it is all in one place, it will be patched much more easier.