toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client
MIT License
1.88k stars 366 forks source link

[feature] delayed_sidekiq strategy #869

Closed skcc321 closed 1 year ago

skcc321 commented 1 year ago

What does it do?

I added a new strategy - delayed_sidekiq. It behaves the next way:

an example:

class CitiesIndex < Chewy::Index
  index_scope City

  strategy_config delayed_sidekiq: {
    latency: 10,  
    margin: 2,
    reindex_wrapper: ->(&reindex) { ActiveRecord::Base.connected_to(role: :reading) { reindex.call }} # fetch data from read replica instead of the primary node
  }

  field :name
  field :address
  ...
end

class City < ActiveRecord::Model
  update_index("cities") { self if persistently_changed? }
  ...
end

------ Request 1 -------
Chewy.strategy(:delayed_sidekiq) do
  City.create(name: "Amsterdam") # created city with id=1
end
------ Request 2 -------
Chewy.strategy(:delayed_sidekiq) do
  City.create(name: "Paris") # created city with id=2
end
------ Request 3 -------
Chewy.strategy(:delayed_sidekiq) do
  City.create(name: "Rome") # created city with id=3
end

those three parallel requests that happened during the "latency" window are going to schedule a single worker to call import with all accumulated ids 1, 2, 3

CityIndex.import([1, 2, 3]) # sql request will go to { role: :reading } due to definition in the index

Strategy option in #import method.

CityIndex.import([1, 2, 3], strategy: :delayed_sidekiq)

Accumulation of :update_fields option between chunks.

CityIndex.import([1, 2], update_fields: ["name"], strategy: :delayed_sidekiq) 
CityIndex.import([3], update_fields: ["address"], strategy: :delayed_sidekiq)

will cause

CityIndex.import([1, 2, 3], update_fields: ["name", "address"])

that can be very useful if you have some complex crutches in the index definition and you don't want to load the database with obviously useless SQL queries.

Configuration


Before submitting the PR make sure the following are checked:

skcc321 commented 1 year ago

@konalegi could you please review this one please? (and trigger the CI)

skcc321 commented 1 year ago

@konalegi one more time, please. Got failed CI due to no Redis service running.

konalegi commented 1 year ago

@skcc321 Thank you very much for the PR, I'll try to review this in a week or find someone who can do it.

skcc321 commented 1 year ago

@konalegi one more time CI, please (I hope the last one) - fixed linters & ruby 2.x related failed tests.

skcc321 commented 1 year ago

@konalegi sorry for bothering you. Any updates on that one?

konalegi commented 1 year ago

@skcc321, sorry, everyone is busy these days. I'll try to review this on Thursday/Friday, added my Todo list

skcc321 commented 1 year ago

@mrzasa thank you for the review. I pushed all suggested changes in a separate commit (just for easier review next time). Could you take a look one more time and restart the CI, please? I'll squash all commits when the review is done and there are no more comments.

skcc321 commented 1 year ago

@konalegi sorry for bothering you, could you please trigger the CI again?

skcc321 commented 1 year ago

@konalegi @mrzasa any updates?

skcc321 commented 1 year ago

Thank you @konalegi. I'll work on your comments soon.

skcc321 commented 1 year ago

@konalegi could you please trigger the CI and look through the latest changes? Thank you

konalegi commented 1 year ago

LGTM, please address my last comment and rubocop offense. Thanks for great feature!

skcc321 commented 1 year ago

@konalegi done. Please take a look at the latest commit.

skcc321 commented 1 year ago

@mrzasa could approve please (if you have no concerns) :)

konalegi commented 1 year ago

Since this PR doesn't modify any existing functionality and brings new feature going to be merge right away. Will be released on Monday

konalegi commented 1 year ago

Released