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

[feature] delayed_sidekiq strategy #869

Merged
merged 8 commits into from
Mar 31, 2023

Conversation

skcc321
Copy link
Contributor

@skcc321 skcc321 commented Jan 12, 2023

What does it do?

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

  • the strategy has a latency option (for instance 10 seconds), margin & reindex_wrapper
  • when you mutate active record objects very frequently the strategy will accumulate ids of mutated records during the latency window.
  • that means that we are going to reindex all changes that happened during 10 seconds at once.
  • such a latency gives us the opportunity to use "read replica" instead of "primary" DB (RoR multi DB connection)
  • as a result, if you don't need strong consistency you can define an appropriate latency window and gain significant performance improvement (fewer calls to DB & replica usage)
  • margin option is important to cover replication_lag

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

  • at CityIndex level
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
 }
  • at the initializer level
Chewy.settings = {
  strategy_config: {
    delayed_sidekiq: {
      reindex_wrapper: ->(&import) { ActiveRecord::Base.connected_to(role: :reading) { import.call } },
      margin: 2, # replication lag seconds
      latency: 10 # accumulation window seconds
    }
  }
}
  • at chewy.yml level
strategy_config:
  delayed_sidekiq:
     latency: 10
     margin: 10
     # reindex_wrapper option is not supported in YAML config. Use either initializer either index file instead.

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

@skcc321 skcc321 marked this pull request as draft January 12, 2023 12:45
@skcc321 skcc321 force-pushed the master-delayed-sidekiq branch 6 times, most recently from 94605b6 to 80019cd Compare January 17, 2023 05:35
@skcc321 skcc321 marked this pull request as ready for review January 25, 2023 15:10
@skcc321 skcc321 changed the title [+] delayed_sidekiq strategy [FEATURE] delayed_sidekiq strategy Jan 25, 2023
@skcc321 skcc321 changed the title [FEATURE] delayed_sidekiq strategy [feature] delayed_sidekiq strategy Jan 25, 2023
@skcc321
Copy link
Contributor Author

skcc321 commented Jan 25, 2023

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

@skcc321 skcc321 force-pushed the master-delayed-sidekiq branch 2 times, most recently from d938e5e to 921e05c Compare January 26, 2023 08:45
@skcc321
Copy link
Contributor Author

skcc321 commented Jan 26, 2023

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

@konalegi
Copy link
Contributor

@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
Copy link
Contributor Author

skcc321 commented Jan 26, 2023

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

@skcc321
Copy link
Contributor Author

skcc321 commented Feb 6, 2023

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

@konalegi
Copy link
Contributor

konalegi commented Feb 6, 2023

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

Copy link
Contributor

@mrzasa mrzasa left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I've added some early comments, I didn't have time to test it properly.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
spec/chewy/strategy/delayed_sidekiq_spec.rb Outdated Show resolved Hide resolved
@skcc321 skcc321 force-pushed the master-delayed-sidekiq branch 2 times, most recently from 387181c to 7fce23a Compare February 8, 2023 12:19
@skcc321
Copy link
Contributor Author

skcc321 commented Feb 8, 2023

@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 skcc321 force-pushed the master-delayed-sidekiq branch 3 times, most recently from 87b13a6 to d1475da Compare February 9, 2023 15:28
@skcc321
Copy link
Contributor Author

skcc321 commented Feb 13, 2023

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

@skcc321
Copy link
Contributor Author

skcc321 commented Feb 24, 2023

@konalegi @mrzasa any updates?

Copy link
Contributor

@konalegi konalegi left a comment

Choose a reason for hiding this comment

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

Sorry for making you wait for so long.
Looks good to me, I'll need to do some manual testing, if everything will be nice, I'll merge it.

lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
lib/chewy/strategy/delayed_sidekiq.rb Outdated Show resolved Hide resolved
@skcc321
Copy link
Contributor Author

skcc321 commented Mar 20, 2023

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

@skcc321
Copy link
Contributor Author

skcc321 commented Mar 21, 2023

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

@konalegi
Copy link
Contributor

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

@skcc321
Copy link
Contributor Author

skcc321 commented Mar 28, 2023

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

README.md Outdated Show resolved Hide resolved
@skcc321
Copy link
Contributor Author

skcc321 commented Mar 29, 2023

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

@konalegi
Copy link
Contributor

konalegi commented Mar 31, 2023

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 konalegi merged commit 8ea2cfe into toptal:master Mar 31, 2023
@konalegi
Copy link
Contributor

konalegi commented Apr 3, 2023

Released

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