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

Reduce handler size in some ActiveJob cases, and fix deserialization inconsistency #24

Merged
merged 7 commits into from
Jan 20, 2023

Conversation

smudge
Copy link
Member

@smudge smudge commented Jan 20, 2023

This should address #23.

The issue here is that some ActiveJob-enqueued jobs would include the @job ivar in their serialized handler, which is redundant with @job_data and only existed for memoization purposes. We can use Ruby's encode_with API to ensure that we only encode @job_data into the handler.

Including @job also had the side-effect of causing DeserializationErrors and permanently failing jobs in cases where there would otherwise be a retryable NameError. Whether a missing constant should result in a DeserializationError is kind of a separate question, but since ActiveJob's stance seems to be to encode the class name as a string and only .constantize it inside of the perform, I think that it's not surprising behavior to let the NameError (and subsequent retries) occur. DeserializationError is really supposed to be only for cases where we fundamentally can't even attempt the job and allow the normal pattern of exception-handling to occur.

@smudge smudge requested a review from effron January 20, 2023 19:42
@smudge
Copy link
Member Author

smudge commented Jan 20, 2023

/no-platform

effron
effron previously approved these changes Jan 20, 2023
Copy link
Contributor

@effron effron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me! Only suggestion would be to add a test that explicitly makes sure we raise a NameError and not a DeserializationError in the circumstances described here #23

@betterment-policy-bot-production betterment-policy-bot-production bot dismissed effron’s stale review January 20, 2023 19:54

Dismissed because the approval didn't include a valid comment pattern

@effron effron self-requested a review January 20, 2023 19:54
effron
effron previously approved these changes Jan 20, 2023
Copy link
Contributor

@effron effron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domainLGTM

Copy link
Contributor

@effron effron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

domainLGTM!

@smudge smudge merged commit 66125d1 into main Jan 20, 2023
@smudge smudge deleted the handler-fix branch January 20, 2023 22:02
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

2 participants