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

Keep executions for each specific exception #34352

Merged

Conversation

Projects
None yet
5 participants
@albertoalmagro
Copy link
Contributor

commented Oct 30, 2018

Summary

ActiveJob used the global executions counter to control the number of times a job should be retried. The problem with this approach was that in case a job raised different exceptions during its executions they weren't retried the number of times defined by their attemps number.

Example:

Having the following job:

class BuggyJob < ActiveJob::Base
  retry_on CustomException, OtherException, attempts: 3
end

If the job raised CustomException in the first two executions and then it raised OtherException, the job wasn't retried anymore because the global executions counter was already indicating 3 attempts.

With this patch each exception has its specific counter so that the first two executions that raise CustomException don't affect the retries count that future exceptions may have.

Other Information

Fixes #34337

r? @rafaelfranca
/cc @dhh @kirs

@lsylvester

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

If the exceptions that are being raised are different subclasses of the exception listed in retry_on then I would expect that they should all be using the same counter - so I am not sure that the error class name is a good key for the counters. Maybe we could use exceptions (from listing in retry_on call) instead, or call rescue_from for each exception in exceptions and use each exception as the key.

EDIT: From #34337 (comment) there should be one counter per declaration so I think exceptions as the key is the way to go.

ie

class CustomError < StandardError; end
class CustomerError1 < CustomerError; end
class CustomerError2 < CustomerError; end

rescue_from CustomError, attempts: 2

# then in job
raise CustomerError1 #=> should retry 

# and on the retry
raise CustomerError2 #=> we should not retry any more.

When there are multiple calls to retry on they should defiantly have seperate counters - but if multiple exceptions are listed maybe they should all go to the same counter.

So

  retry_on CustomException, OtherException, attempts: 3

could use a single counter but

  retry_on CustomException, attempts: 3
  retry_on OtherException, attempts: 3

could use seperate counters.

@saiqulhaq

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@lsylvester

So retry_on CustomException, OtherException, attempts: 3
could use a single counter but
retry_on CustomException, attempts: 3
retry_on OtherException, attempts: 3
could use seperate counters.

not easy to be distinguished, is it using same or separate counters if we haven't read the docs

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

@lsylvester thanks for your comment and suggestions. As @saiqulhaq points out, I couldn't find in the current documentation any clue that subclasses of the exception listed in retry_on should be also retried, or that attempts should be counted for each declaration, but I would be more than happy to adapt this PR and the current implementation to have a behavior that pleases everyone.

@dhh could you please clarify if the following points that @lsylvester proposes is the way this was intended to work? To summarize them:

  1. Subclasses of the exceptions listed in retry_on declarations should be also retried.
  2. Attempts should not be counted for each exception separately but for each retry_on declaration.

Thanks in advance!

@dhh

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

Yes, the intent is to count exclusively on a per retry_on declaration. Not on the class hierarchy of the exception.

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

Thanks, I'll change it then!

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:dedicated-execution-counter-for-each-exception branch from 3007e61 to c6b51eb Nov 4, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2018

@dhh @lsylvester Feedback applied. Now it counts exclusively on a per retry_on declaration and not on the class hierarchy of the exception, as originally intended.

I have also reverted 86aa8f8 as in my opinion it applied to a global single executions counter and now it would be misleading.

@dhh

This comment has been minimized.

Copy link
Member

commented Nov 4, 2018

At a cursory look, this is 👌. Maybe Rafael or someone else can do a deeper implementation review.

Will also need a solid change log entry. This is a kind of breaking change (though one I’m perfectly comfortable with for 6.0 without deprecation or whatever).

Thanks for your work on this! ❤️

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:dedicated-execution-counter-for-each-exception branch from c6b51eb to 1c72661 Nov 5, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

@dhh my pleasure!

Change log entry added and squashed into the same commit.

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:dedicated-execution-counter-for-each-exception branch from 1c72661 to 58ce3e1 Nov 7, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@rafaelfranca as one CI job didn't start I took the chance to rebase this PR. Now CI was successful and the PR is neat and pretty for review.

albertoalmagro added some commits Oct 30, 2018

Keep executions for each specific declaration
Fixes #34337

ActiveJob used the global executions counter to control the number of
times a job should be retried. The problem with this approach was that
in case a job raised different exceptions during its executions they
weren't retried the number of times defined by their `attemps` number.

**Example:**

Having the following job:
```ruby
class BuggyJob < ActiveJob::Base
  retry_on CustomException, attemps: 3
  retry_on OtherException, attempts: 3
end
```
If the job raised `CustomException` in the first two executions and then
it raised `OtherException`, the job wasn't retried anymore because the
global executions counter was already indicating 3 attempts.

With this patch each `retry_on` declaration has its specific counter so
that the first two executions that raise `CustomException` don't affect
the retries count that future exceptions may have.

@albertoalmagro albertoalmagro force-pushed the albertoalmagro:dedicated-execution-counter-for-each-exception branch from 58ce3e1 to 875809d Nov 23, 2018

@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Rebased to resolve conflicts in activejob/CHANGELOG.md.

Review please 🙏

@dhh dhh merged commit 95d9c3b into rails:master Nov 23, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@albertoalmagro

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Thanks!!! ❤️

mrhead added a commit to mrhead/rails that referenced this pull request Apr 12, 2019

Use individual execution counters when calculating retry delay
Individual execution targets were introduced in rails#34352. However
`#determine_delay` which is used to calculate retry delay still uses the
global counter. This commit fixes it.

mrhead added a commit to mrhead/rails that referenced this pull request Apr 12, 2019

Use individual execution counters when calculating retry delay
Individual execution counters were introduced in rails#34352. However
`#determine_delay` which is used to calculate retry delay still uses the
global counter. This commit fixes it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.