Skip to content

Conversation

@yelled3
Copy link
Contributor

@yelled3 yelled3 commented May 27, 2014

@seuros take a look

I'm starting to thing this split is an overkill - mostly the ManagerWorker part...
maybe we should just have a gem per-ORM, with some duplications

WDYT?

@seuros
Copy link
Member

seuros commented May 28, 2014

I will take a look at this today and come back to you. Sorry for delay.

@yelled3
Copy link
Contributor Author

yelled3 commented May 28, 2014

@seuros no worries, take your time

@yelled3
Copy link
Contributor Author

yelled3 commented May 28, 2014

perform_bulk will execute N time UserMailerWorker.perform_async

the reason I use Sidekiq::Client.push_bulk is because doing perform_async N times is costly, when N is large.

but other then that perform_bulk is just an alias of perform_query_async, no?

@yelled3
Copy link
Contributor Author

yelled3 commented May 28, 2014

we can use Activesupport::Concern

I'm trying to minimize dependencies, just like sidekiq is doing:

https://github.com/mperham/sidekiq/blob/ddb58482cfe6b934d632b25c560298c8da4838d8/lib/sidekiq/core_ext.rb#L5

@seuros
Copy link
Member

seuros commented May 28, 2014

I'm trying to minimize dependencies

You can't, ActiveRecord already loaded ActiveSupport.

@seuros
Copy link
Member

seuros commented May 28, 2014

but other then that perform_bulk is just an alias of perform_query_async, no?

yes

the reason I use Sidekiq::Client.push_bulk is because doing perform_async N times is costly, when N is large.

That why we should do test with live data.

@yelled3
Copy link
Contributor Author

yelled3 commented May 28, 2014

That why we should do test with live data.

there's no need to benchmark it - already been done. see:

http://stackoverflow.com/a/20692081
http://www.rubydoc.info/github/mperham/sidekiq/Sidekiq/Client.push_bulk

@yelled3
Copy link
Contributor Author

yelled3 commented Jun 5, 2014

continued in: #15

@yelled3 yelled3 closed this Jun 5, 2014
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