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

Add SQS for active job #21406

Closed
wants to merge 1 commit into from
Closed

Conversation

sharmaansh21
Copy link
Contributor

This is still work in progress. But before i complete it i wanna know what others think about it.

@sharmaansh21
Copy link
Contributor Author

/cc @rafaelfranca

class SqsAdapter
def enqueue(job) #:nodoc:
JobWrapper.queue_as job.queue_name
ActiveSupport::JSON.encode(job.serialize)
Copy link
Member

Choose a reason for hiding this comment

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

debug code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seuros It's just some testing i was doing, but before i go ahead and complete this feature, i wanna know what others think about having Amazon SQS as adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seuros yup sort of.

@@ -202,6 +208,10 @@ GEM
mysql2 (0.3.19)
nokogiri (1.6.6.2)
mini_portile (~> 0.6.0)
nokogiri (1.6.6.2-x64-mingw32)
Copy link
Member

Choose a reason for hiding this comment

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

why this was added ?

@seuros
Copy link
Member

seuros commented Aug 27, 2015

If the adapter is 100% compatible with the others adapters, i'm 👍 to add it to Active Job 5.0.

ping @dhh

@sharmaansh21 sharmaansh21 changed the title Added SQS for active job Add SQS for active job Aug 27, 2015
@sharmaansh21
Copy link
Contributor Author

Yes it is 100% compatible with the other adapters.

register_worker!(job)

delay = (timestamp - Time.current.to_f).round
raise 'The maximum allowed delay is 15 minutes' if delay > 15.minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

could raise ArgumentError

Copy link
Contributor

Choose a reason for hiding this comment

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

also, perhas move 15.minutes to a constant

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this should be documented like this.

@rafaelfranca
Copy link
Member

I remember I was against adding a lot of adapters inside Rails when we were discussion about Active Job. For me it should be the library authors job to maintain their Active Job adapters. I'm fine with adding some adapters by default but this is already causing maintenance burden since Active Job tests is always failing because something on the libraries.

I really believe we should stop including new adapters and encourage library author to maintain their adapters.

@dhh @jeremy what do you think?

@rafaelfranca rafaelfranca self-assigned this Sep 9, 2015
@kuldeepaggarwal
Copy link
Contributor

Yes, this is also a great idea that we have different adapters with different authors to maintain it, it will distribute the responsibility. 😄

@rafaelfranca
Copy link
Member

See #23311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants