Migrate search to Searchkick gem. by simi · Pull Request #3199 · rubygems/rubygems.org

simi · 2022-08-27T21:26:19Z

removes elasticsearch-rails dependency not supporting opensearch
exactly same index is used, there is no need for reindex in production/staging

ℹ️ Once we migrate to searchkick (or fix the non-existing opensearch-rails gem problem), I can continue on upgrading tech stack at AWS to latest OpenSearch version.

simi · 2022-08-27T21:30:54Z

 gem "unicorn", "~> 5.8.0"
 gem "validates_formatting_of"
 gem "elasticsearch", "~> 7.10.1"
-gem "elasticsearch-api", "~> 7.10.1"


I have removed explicit api/transport requirement since we don't use it directly and it is already dependency of elasticsearch gem.

codecov · 2022-08-27T21:31:48Z

Codecov Report

Merging #3199 (4ce8dc5) into master (84ece6b) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 4ce8dc5 differs from pull request most recent head 6fca499. Consider uploading reports for the commit 6fca499 to get more accurate results

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
- Coverage   98.52%   98.45%   -0.07%     
==========================================
  Files         113      110       -3     
  Lines        3390     3302      -88     
==========================================
- Hits         3340     3251      -89     
- Misses         50       51       +1

Impacted Files	Coverage Δ
app/models/concerns/rubygem_searchable.rb	`100.00% <100.00%> (+5.55%)`	⬆️
app/models/deletion.rb	`100.00% <100.00%> (ø)`
app/models/gem_download.rb	`100.00% <100.00%> (ø)`
app/models/pusher.rb	`94.17% <100.00%> (ø)`
lib/elastic_searcher.rb	`100.00% <100.00%> (ø)`
app/controllers/api/v1/api_keys_controller.rb	`94.11% <0.00%> (-5.89%)`	⬇️
lib/fastly.rb	`100.00% <0.00%> (ø)`
app/models/user.rb	`100.00% <0.00%> (ø)`
app/controllers/sessions_controller.rb	`100.00% <0.00%> (ø)`
app/controllers/settings_controller.rb	`100.00% <0.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

simi · 2022-08-27T21:32:12Z

  end

  def update_search_index
-    @version.rubygem.delay.update_document


Is there any alternative to update_document in searchkick @ankane?

It's not exactly the same, but there is partial reindexing.

simi · 2022-08-27T21:38:00Z

@epugh @ankane thanks for the recommendation and all the work on Searchkick. Would you mind to do review as well? 🙏 I did my best to follow documentation.

simi · 2022-08-27T21:45:48Z

+    Rubygem.searchkick_index.create
  end

-  task :import_alias do


Online async import is supported in Searchkick by default https://github.com/ankane/searchkick#parallel-reindexing, but I wasn't able to setup it locally without adding Redis dependency. Anyway since we don't need reindexing currently, I just made it simple.

In production we can call:

index = Rubygem.reindex(mode: :async) # wait once done Rubygem.searchkick_index.promote(index[:index_name])

We used this task for upgrades (which required new index for backward incompatible change) or fix/add some data from our end.
Creating a new index and updating the alias allowed us to do this without downtime. Creating new index takes considerable amount of time.

As I mentioned, we can do the same and it is built in. Searchkick utilizes ActiveJob we doesn't really use currently. But I have plan to move all .delay calls to proper background jobs soon as well. For now we can do async reindexing as mentioned. Not reindexing is needed when deployed since the using index is exactly the same (checked manually via API).

For now we can do async reindexing as mentioned.

Does this work without redis? I think somewhere you commented that async needs redis.

Yes, redis is needed just for optional progress tracking AFAIK.

ankane

Hey @simi, left some comments inline, but overall, looks good from a Searchkick standpoint!

ankane · 2022-08-28T17:58:19Z

  end

  def update_search_index
-    @version.rubygem.delay.update_document


It's not exactly the same, but there is partial reindexing.

ankane · 2022-08-28T18:41:50Z

    end
    Delayed::Job.enqueue Indexer.new, priority: PRIORITIES[:push]
-    rubygem.delay.index_document
+    rubygem.delay.reindex


fwiw, delay.reindex isn't tested in Searchkick (the recommended approach is reindex(mode: :async))

This just calls rubygem.reindex in backround job. That should be safe.

We don't use ActiveJob for now, but I have plan to migrate from delay job being used directly to ActiveJob. Once done, we can move to reindex(mode: :async) if I understand it well.

ankane · 2022-08-28T19:22:04Z

Also, forgot to mention Searchkick has built-in Rake tasks for reindexing, you might be able to remove elasticsearch.rake.

rake searchkick:reindex:all
# or
rake searchkick:reindex CLASS=Rubygem

simi · 2022-09-10T22:29:00Z

@ankane would you mind to do another review round once you have some spare time 🙏?

ankane · 2022-09-11T19:21:56Z

Hey @simi, looks great!

While not needed with the current code (since it only calls response), you may want to pass load: false to searchkick_search calls to ensure records won't get loaded from the DB if someones tries to use a different method on results in the future. https://github.com/ankane/searchkick/#results

simi · 2022-10-29T16:54:04Z

This is ready from my side @sonalkr132 @jenshenny. Next step would be to merge and test on staging.

sonalkr132 · 2022-11-01T13:14:17Z

@@ -482,10 +480,7 @@ DEPENDENCIES
  delayed_job
  delayed_job_active_record
  elasticsearch (~> 7.10.1)


Do we have plans to remove this? I am guessing we can't upgrade our cluster as long as we have this in out gemfile.

Yes, plan is to move to OpenSearch gems once this lands to production.

Can you please elaborate on the order of changes?

deploy this to prod

replace elasticsearch in gemfile with opensearch (does opensearch gem work with ES cluster)

upgrade ES cluster engine in staging to opensearch

Is this the plan?

Once this will be merged, deployed to staging and considered safe to deploy to production, we can deploy to production.

After that my plan is to open other PR moving out of elasticsearch gem (migrate to opensearch). That can be merged and tested on staging as well. There is no need for upgrade of ES cluster.

And again after some time we can finally upgrade ES cluster engine. First I would like to try on staging to see if any downtime is needed and get some estimate on time needed for upgrade in general and repeat in production. If I understand it well, application works well without ES. The only problem could be unindexed changes. We can track gems recently changed (during maintenance window) and explicitly ask for reindex for those.

That's my initial plan.

jenshenny

Thanks @simi! I added a couple of comments but generally it looks good to me.

jenshenny

🚀

simi · 2022-11-16T20:46:24Z

I have tested this on staging (manually deploying this branch in there). Everything works well with no needed for reindexing. I have tested reindexing as well.

rake task

ℹ️ this wipes the index first so search is affected by this

time kubectl exec -it deploy/unicorn -n rubygems-staging --context rubygems -- bin/rake searchkick:reindex CLASS=Rubygem
Defaulted container "unicorn" out of: unicorn, nginx
** [Honeybadger] Initializing Honeybadger Error Tracker for Ruby. Ship it! version=5.0.2 framework=rails level=1 pid=32
Reindexing Rubygem...
Reindex successful

real    40m21,265s
user    0m1,088s
sys     0m0,490s

manual async

ℹ️ is needed to be merged #3266 for this

ℹ️ this is not affecting search functionality

# schedule reindex in batches using ActiveJob
index = Rubygem.reindex(mode: :async)
#=> {:index_name=>"rubygems-staging_20221113223835766"}

# check progress
Delayed::Backend::ActiveRecord::Job.where(queue: 'searchkick').count
#=> 175

# switch
Rubygem.searchkick_index.promote(index[:index_name])
#=> {"acknowledged"=>true}

simi · 2022-11-16T20:51:20Z

I think this is ready for merge.

- removes elasticsearch-rails dependency not supporting opensearch

deivid-rodriguez · 2022-12-31T08:48:56Z

🙌

simi requested review from jenshenny and sonalkr132 August 27, 2022 21:26

simi commented Aug 27, 2022

View reviewed changes

Comment thread config/application.rb

simi commented Aug 27, 2022

View reviewed changes

Comment thread test/integration/advanced_search_test.rb

simi commented Aug 27, 2022

View reviewed changes

Comment thread test/test_helper.rb Outdated

simi force-pushed the searchkick branch from dc2b9b8 to d7fdc2f Compare August 27, 2022 21:35

simi commented Aug 27, 2022

View reviewed changes

simi marked this pull request as ready for review August 27, 2022 21:45

ankane reviewed Aug 28, 2022

View reviewed changes

simi force-pushed the searchkick branch 4 times, most recently from 432dcff to 87a70e3 Compare September 10, 2022 22:15

simi mentioned this pull request Sep 27, 2022

Proposal: Fork and maintain elasticsearch-rails opensearch-project/opensearch-ruby#50

Closed

simi force-pushed the searchkick branch 3 times, most recently from d1c8609 to 49676f4 Compare October 29, 2022 15:56

simi requested a review from ankane October 29, 2022 16:53

sonalkr132 reviewed Nov 1, 2022

View reviewed changes

jenshenny reviewed Nov 2, 2022

View reviewed changes

Comment thread lib/tasks/elasticsearch.rake

Comment thread test/helpers/es_helper.rb Outdated

Comment thread test/unit/rubygem_searchable_test.rb Outdated

Comment thread lib/tasks/elasticsearch.rake

simi requested review from jenshenny and removed request for ankane November 5, 2022 00:15

sonalkr132 approved these changes Nov 5, 2022

View reviewed changes

jenshenny approved these changes Nov 7, 2022

View reviewed changes

simi requested a review from sonalkr132 November 17, 2022 12:50

simi force-pushed the searchkick branch from 4ce8dc5 to cdad61e Compare November 17, 2022 12:52

simi mentioned this pull request Nov 20, 2022

Use DelayedJob as active job adapter. #3266

Merged

Migrate search to Searchkick gem.

6fca499

- removes elasticsearch-rails dependency not supporting opensearch

simi force-pushed the searchkick branch from d8a49c7 to 6fca499 Compare December 15, 2022 00:23

martinemde mentioned this pull request Dec 23, 2022

Bump elasticsearch-rails to 7.2.1 for import fix (but blocks using opensearch) #3299

Closed

simi merged commit 01ee51b into master Dec 30, 2022

simi deleted the searchkick branch December 30, 2022 22:07

rubygems-org-shipit Bot temporarily deployed to staging December 30, 2022 23:06 Inactive

rubygems-org-shipit Bot temporarily deployed to production December 30, 2022 23:10 Inactive

Uh oh!

Uh oh!

Conversation

simi commented Aug 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simi commented Aug 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ankane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ankane commented Aug 28, 2022

Uh oh!

simi commented Sep 10, 2022

Uh oh!

ankane commented Sep 11, 2022

Uh oh!

simi commented Oct 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenshenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jenshenny left a comment

Choose a reason for hiding this comment

Uh oh!

simi commented Nov 16, 2022

Uh oh!

simi commented Nov 16, 2022

Uh oh!

deivid-rodriguez commented Dec 31, 2022

Uh oh!

Reviewers

codecov Bot commented Aug 27, 2022 •

edited

Loading