Skip to content

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

Closed
martinemde wants to merge 3 commits into
masterfrom
martinemde/elasticsearch-import-fix
Closed

Bump elasticsearch-rails to 7.2.1 for import fix (but blocks using opensearch)#3299
martinemde wants to merge 3 commits into
masterfrom
martinemde/elasticsearch-import-fix

Conversation

@martinemde

@martinemde martinemde commented Dec 15, 2022

Copy link
Copy Markdown
Contributor

Fixes a problem with running the elasticsearch import rake task. ActiveRecord find_in_batches api requires being called with kwargs, but elasticsearch-rails was only fixed to send kwargs properly in 7.2.1.

@martinemde

Copy link
Copy Markdown
Contributor Author

Looking at the test failure.

@codecov

codecov Bot commented Dec 15, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3299 (fa638d2) into master (ba6d6e1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3299   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files         113      113           
  Lines        3390     3390           
=======================================
  Hits         3340     3340           
  Misses         50       50           

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

@martinemde

martinemde commented Dec 16, 2022

Copy link
Copy Markdown
Contributor Author

Failure is test order dependent which is a little troubling. The failure looks possibly related to the elasticsearch and faraday upgrade, but doesn't fail unless you use the seed bin/rails test --seed 51142.

EDIT: Even more confusing, my first run with that seed failed, the second run with that same seed passed.

@martinemde

martinemde commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

I think this bug only happens when the tests try to refresh an index that doesn't exist. The failure happened when trying to trigger logging before suppressing the exception.

I was able to reproduce the failure one time when I did gem update --system before running the tests, then it only happens the first time.

Since the default behavior in elasticsearch-model is to ignore the missing index on refresh_index, I think this was always happening. It just wasn't triggering any failures since the code in the rescue executed properly. I think it's safe to assume this failure is gone and the tests will behave the same as before now.

Fixes a failure that blocked running the elasticsearch import rake task
caused by ActiveRecord find_in_batches api change.

elasticsearch-model 7.2.1 relies on an undocumented internal api change
that requires elasticsearch and elasticsearch-transport to be updated at
the same time as elasticsearch-rails and elasticsearch-model.

Fix our elasticsearch setup for the api change from `client.transport` to
`client.transport.transport`.
@martinemde martinemde force-pushed the martinemde/elasticsearch-import-fix branch from f2dbc41 to 374368d Compare December 20, 2022 00:53
@martinemde martinemde changed the title Bump elasticsearch-rails/-model to 7.2.1 for compatibility Bump elasticsearch* gems to 7.17 and 7.2.1 for newest rails compatibility Dec 20, 2022
@martinemde

Copy link
Copy Markdown
Contributor Author

I fixed the index not existing in #3303.

@martinemde

Copy link
Copy Markdown
Contributor Author

Hey @simi, it looks like you've done a lot of work with elasticsearch and searchkick. Replacing the import task referenced in the contributing docs (that is currently broken) is my main goal here. If #3199 is merged, searchkick replaces that command with a searchkick command. Can I help you get that across the finish line or should we do the minor upgrade here first?

@simi

simi commented Dec 23, 2022

Copy link
Copy Markdown
Contributor

@martinemde I do plan to merge Searchkick soon. Are those gems in this update still OpenSearch compatible?

@simi

simi commented Dec 23, 2022

Copy link
Copy Markdown
Contributor

It seems we can't upgrade after this elastic/elasticsearch-ruby#1360 released with 7.14.

@martinemde

martinemde commented Dec 27, 2022

Copy link
Copy Markdown
Contributor Author

Pushed a version locked to 7.13.3 (before the opensearch breaking change mentioned above).

@martinemde

martinemde commented Dec 27, 2022

Copy link
Copy Markdown
Contributor Author

😬 Looks like the ruby 3 compatible version of elasticsearch-rails (7.2.1) uses transport.transport which is not supported in 7.13.

It seems to me that if we are switching to opensearch, then we need a replacement version for open search or we have to use older versions and monkeypatch the kwargs incompatibility.

@martinemde martinemde marked this pull request as draft December 27, 2022 20:44
@martinemde martinemde changed the title Bump elasticsearch* gems to 7.17 and 7.2.1 for newest rails compatibility Bump elasticsearch-rails to 7.2.1 for import fix (but blocks using opensearch) Dec 27, 2022
@martinemde

Copy link
Copy Markdown
Contributor Author

I reverted to use 7.17, since it's simply broken on 7.13.3. I converted this to a draft because #3199 may inform how we progress and we may want to decide before merging this.

@simi

simi commented Dec 30, 2022

Copy link
Copy Markdown
Contributor

elasticsearch-rails was removed at 01ee51b#diff-89cade48462044ee1b672dc5f4c3ec250fbd29effcd8932096a23c1283c6731fL158, closing

@simi simi closed this Dec 30, 2022
@simi simi deleted the martinemde/elasticsearch-import-fix branch December 30, 2022 23:19
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.

2 participants