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

Infer interruption adapter from queue adapter, take 2 #450

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

Mangara
Copy link
Contributor

@Mangara Mangara commented Jan 11, 2024

This PR is an attempt to redo #367 in a backwards compatible way. That will allow us to release this as 1.x, and remove the deprecations in 2.0.

Future behaviour

Like that PR, the desired interface going forward is:

  • Interruption adapters get registered (automatically for ones that job-iteration knows about, manually for others):
# happens automatically if resque is available
JobIteration::InterruptionAdapters.register(:resque, JobIteration::InterruptionAdapters::ResqueAdapter) 

# the default can be overridden
JobIteration::InterruptionAdapters.register(:sidekiq, MyCustomSidekiqAdapter) 

# custom interruption adapter for queue adapters that job-iteration doesn't know about
JobIteration::InterruptionAdapters.register(:awesome_queue, AwesomeQueueAdapter) 
  • At runtime, job-iteration looks up the interruption adapter based on the queue_adapter_name of the job class.
def interruption_adapter
  JobIteration::InterruptionAdapters.lookup(queue_adapter_name)
end

Backwards compatibility

The major differences are that:

  • At lookup time, if JobIteration.interruption_adapter is set, we use that instead:
def interruption_adapter
  JobIteration.interruption_adapter || JobIteration::InterruptionAdapters.lookup(queue_adapter_name)
end
  • Instead of removing JobIteration.interruption_adapter=, we print a deprecation warning:
> JobIteration.interruption_adapter = Foo.new
DEPRECATION WARNING: Setting JobIteration.interruption_adapter is deprecated. Use JobIteration::InterruptionAdapters.register(:foo, callable) instead to register the callable (a proc, method, or other object responding to #call) as the interruption adapter for queue adapter :foo.
  • Instead of raising when we don't find a matching queue adapter (and JobIteration.interruption_adapter is not set), we log a deprecation warning:
DEPRECATION WARNING: No interruption adapter is registered for :fancy_queue; falling back to `NullAdapter`, which never interrupts.
Use `JobIteration::InterruptionAdapters.register(:fancy_queue, <adapter>) to register one.
This will raise starting in version 2.0 of JobIteration!"

@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch from f0fbea8 to ffe6d9b Compare January 24, 2024 19:40
@Mangara Mangara changed the title Mangara multi interruption adapter 2 Support multiple interruption adapters Jan 24, 2024
@Mangara Mangara changed the title Support multiple interruption adapters Infer interruption adapter from queue adapter, take 2 Jan 24, 2024
@Mangara Mangara marked this pull request as ready for review January 24, 2024 21:26
@Mangara
Copy link
Contributor Author

Mangara commented Jan 24, 2024

@bdewater Sorry it's taken us so long to get this change out. I hope this gives us a good way to move forward. I can't seem to tag you as reviewer, but I'd welcome your input.

@Mangara Mangara requested a review from sambostock January 24, 2024 21:30
Copy link
Contributor

@bdewater bdewater left a comment

Choose a reason for hiding this comment

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

Left a few small comments but it's looking good! Thanks @Mangara and @sambostock. Also cc'ing @plasticine for the original PR I based my work on.

lib/job-iteration.rb Outdated Show resolved Hide resolved
test/integration/interruption_adapters_test.rb Outdated Show resolved Hide resolved
lib/job-iteration.rb Show resolved Hide resolved
@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch from 51a7958 to a37aabd Compare January 26, 2024 16:16
require_relative "./job-iteration/iteration"
require_relative "./job-iteration/log_subscriber"
require_relative "./job-iteration/railtie"

module JobIteration
IntegrationLoadError = Class.new(StandardError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this considered part of the public API? Seems unlikely someone would be referencing the constant though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only references I can find in GitHub are RBI files: https://github.com/search?q=JobIteration%3A%3AIntegrationLoadError&type=code
I highly doubt anyone was rescuing this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Good reminder for us to ensure we're masking things as # @api private though, as we are with the built in adapters constant.

lib/job-iteration.rb Outdated Show resolved Hide resolved
test/integration/interruption_adapters_test.rb Outdated Show resolved Hide resolved
test/integration/interruption_adapters_test.rb Outdated Show resolved Hide resolved
@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch 6 times, most recently from 71f045d to 9c13ff9 Compare February 8, 2024 15:46
Comment on lines 78 to 80
def interruption_adapter
JobIteration.interruption_adapter || JobIteration::InterruptionAdapters.lookup(queue_adapter_name)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this method as a class method allowed the job class to overwrite it, which feels like it would be useful in tests, if nothing else (since we plan to get rid of the global JobIteration.interruption_adapter).

By moving the logic into the private method marked as @private, we've made it so it can't be controlled without violating the privacy contract.

Is that something we should reconsider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline, but I feel like for tests we can register a configurable TestAdapter for when the test queue adapter is used. For other cases, I agree there's a missing configuration option, but I'm not convinced the interruption adapter is the right place for that. I like keeping its scope small - to tell us when the job worker is stopping.

@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch from 9c13ff9 to d8cecc7 Compare February 9, 2024 16:57
lib/job-iteration/interruption_adapters/resque_adapter.rb Outdated Show resolved Hide resolved
Comment on lines 65 to 68
Deprecation.warn("Setting JobIteration.interruption_adapter is deprecated."\
" Use JobIteration::InterruptionAdapters.register(:foo, callable) instead"\
" to register the callable (a proc, method, or other object responding to #call)"\
" as the interruption adapter for queue adapter :foo.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I could have sworn there was a cop that forced trailing spaces and spaces before the \...

Suggested change
Deprecation.warn("Setting JobIteration.interruption_adapter is deprecated."\
" Use JobIteration::InterruptionAdapters.register(:foo, callable) instead"\
" to register the callable (a proc, method, or other object responding to #call)"\
" as the interruption adapter for queue adapter :foo.")
Deprecation.warn("Setting JobIteration.interruption_adapter is deprecated. " \
"Use JobIteration::InterruptionAdapters.register(:foo, callable) instead " \
"to register the callable (a proc, method, or other object responding to #call) " \
"as the interruption adapter for queue adapter :foo.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is, LineContinuationLeadingSpace, and it should be enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why it wasn't flagging this, but I fixed the spaces.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, while we do have it enabled on rubocop-shopify@main, it's still disabled in the latest release (v2.14.0), which is what this repo is using.

@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch from d8cecc7 to 0322775 Compare February 9, 2024 19:25
@Mangara Mangara force-pushed the mangara-multi-interruption-adapter-2 branch from 0322775 to db81365 Compare February 9, 2024 21:58
@Mangara Mangara merged commit 1c6ab96 into main Feb 9, 2024
45 checks passed
@Mangara Mangara deleted the mangara-multi-interruption-adapter-2 branch February 9, 2024 22:10
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.

3 participants