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

can_enqueue? propagation detection that does not depend on Rails #1672

Merged
merged 1 commit into from
Sep 9, 2016

Conversation

kares
Copy link
Contributor

@kares kares commented Sep 7, 2016

Rails.configuration in particular ... should be cleaner and easier to test/manage

now also raises when adapter is neither DJ or Resque ... which is desired, right?

@dsander
Copy link
Collaborator

dsander commented Sep 7, 2016

Hi @kares,

raising an exception when the configured adapter is not supported makes total sense.

I don't fully understand why not using Rails.configuration is an advantage. If we would make the specs adapter specific (they aren't at the moment because we do not support rescue officially) we would now need to stub a method instead of changing the configuration for a context/spec.

@kares
Copy link
Contributor Author

kares commented Sep 7, 2016

thanks @dsander ... somehow do not like to depending on Rails API in cases such as these - feels like 'duplication'. Rails already booted/configured all sub-systems such as AR/AM/AC (in case of AJ self.class.queue_adapter is already resolved based on the configuration).

it should not be hard to test, stub-ing shouldn't be necessary since one can direct set queue_adapter=.
actually, should be possible to unit test this piece now outside of Rails (not that I am saying anyone will).

return false
case queue_adapter.name # not using class since it would load adapter dependent gems
when 'ActiveJob::QueueAdapters::DelayedJobAdapter'
if Delayed::Job.where(failed_at: nil, queue: 'propagation').count > 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

return Delayed::Job.where(failed_at: nil, queue: 'propagation').count == 0

and

return Resque.size('propagation') == 0 && Resque.workers.select { |w| w.job && w.job['queue'] && w.job['queue']['propagation'] }.count == 0

That's the same, right?

@dsander
Copy link
Collaborator

dsander commented Sep 8, 2016

@kares Gotcha, thanks for the explanation.

Looks good to me.

@kares
Copy link
Contributor Author

kares commented Sep 9, 2016

updated as suggested. huginn is 🅰️some ... thank you!

@cantino cantino merged commit c6a17c9 into huginn:master Sep 9, 2016
@cantino
Copy link
Member

cantino commented Sep 9, 2016

Thanks!

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

3 participants