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

Chewy / Elastic search version update #26399

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjankowski
Copy link
Contributor

@mjankowski mjankowski commented Aug 8, 2023

As discussed here - #24209 (comment) - we are limited in our ability to update omniauth_openid_connect or anything else which wants to be on Faraday 2+, because chewy gem has dependency on older ES gems which are locked to Faraday 1.x.

Opening this as draft to just get it moving. For now I'm pointing to an experimental branch someone has linked from this issue - toptal/chewy#847 - which has started on some basic class changes in Chewy to support changes made in the ES gems. With that alone and one small change on our side I was able to get things loading/running/passing, but there are a ton of caveats here (hence the draft status!):

  • Similar to the omniauth changes, we don't have exhaustive chewy/ES integration coverage in the specs. Most of what we do have is just stubbing out ES responses with chewy mock helpers and verifying basic chewy behavior. Makes it hard to be confident about results here. We should almost definitely improve this situation before upgrading.
  • How fully does this experimental chewy branch actually do all updates needed for the 8.x ES gems (split into transport and api gems) -- I don't know currently.
  • There’s a warning from the gem during spec run because of error connecting to server and how server is not official supported version. I think this is actually fine to ignore, but in the few mins I looked it didn't seem to be disableable.
  • There's an insistence from the official 8.x server on secure/443/https connections, which our default config does not seem to do. (not relevant: we wont update server to 8.x any time soon)
  • I suspect we have some specs which are not correctly stubbed out … I was able to get different spec output when running -vs- not running an actual ES server locally (fixed in Avoid connecting to a running ES instance in ES search check spec #26413)
  • I did a quick console session to see if Chewy could just connect with both a 7.x and 8.x server running locally and this chewy gem update in place, and was able to at least get that connection established, but have done zero work yet to verify anything else works.
  • I know there are licensing changes on the server side of things either in server v7 or moving from 7 to 8 and have not at all contemplated whether that matters here yet (I think it does not narrowly matter, because we're not changing the packaged Docker ES distribution, but maybe its related). (not relevant: we arent packaging server, and we will only update client libs here)

All that to say, this is currently focused merely on accomplishing the upgrade at all, and needs a lot more verification of behavior (which I’ll continue on) … just wanted to open this to clarify direction here and whether this is a good path to work on (as opposed to investigating a switch to OpenSearch, and/or any other ideas to open up our Faraday update Future).

@renchap renchap added dependencies Pull requests that update a dependency file area/search ruby Pull requests that update Ruby code labels Aug 8, 2023
@renchap
Copy link
Sponsor Member

renchap commented Aug 8, 2023

  • I know there are licensing changes on the server side of things either in server v7 or moving from 7 to 8 and have not at all contemplated whether that matters here yet (I think it does not narrowly matter, because we're not changing the packaged Docker ES distribution, but maybe its related).

IIRC Elastic support versions N and N-1 in their official libs, so we should be able to use their version 8 lib with a v7 server.

The licence change is significant, but in practice it only impacts you if you embed ES, or offer it as a service, which should not impact any Mastodon admin or user.

@mjankowski
Copy link
Contributor Author

IIRC Elastic support versions N and N-1 in their official libs, so we should be able to use their version 8 lib with a v7 server.

The licence change is significant, but in practice it only impacts you if you embed ES, or offer it as a service, which should not impact any Mastodon admin or user.

Thanks, good to know. I'll keep this PR focused on getting the 8.x client libs from the gems working against a running 7.x ES server cluster.

@mjankowski
Copy link
Contributor Author

Rebased against main after the spec stub thing was merged in. Edited original description to strike out some not relevant concerns.

I've done a little more local console testing, and these things seem ok:

  • Enable Chewy/ES with env vars; create a status with some unique text value; run StatusIndex.query(...), confirm status is in results
  • Run bin/tootctl search deploy, output looks fine, ES server logs seem normal during command
  • Use web UI to perform similar experiment -- server running in ES mode is able to search/retrieve relevant accounts/statuses from dev db after being indexed.

Would love ideas here for what else to verify.

@mjankowski mjankowski marked this pull request as ready for review August 14, 2023 13:22
@mjankowski
Copy link
Contributor Author

Converted from Draft to ready for review because I think this might actually be fine.

Would still love ideas for more things to test out (either via added specs or just local console work).

Note that this PR diff currently includes a gem bump to a specicic commit ref on a PR branch, not an official chewy release. Tried to ping chewy maintainers to see if we can help get it updated.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@mjankowski
Copy link
Contributor Author

Bumped to elasticsearch 8.11.0 gem version.

Still pointing to a chewy PR branch, not sure what to do about that - gem appears to be unmaintained maybe?

Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.02%. Comparing base (86500e3) to head (9d2224c).
Report is 241 commits behind head on main.

❗ Current head 9d2224c differs from pull request most recent head 06e6253. Consider uploading reports for the commit 06e6253 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #26399   +/-   ##
=======================================
  Coverage   85.01%   85.02%           
=======================================
  Files        1059     1059           
  Lines       28277    28286    +9     
  Branches     4538     4535    -3     
=======================================
+ Hits        24040    24050   +10     
+ Misses       3074     3073    -1     
  Partials     1163     1163           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Feb 6, 2024

This pull request has resolved merge conflicts and is ready for review.

@mjankowski mjankowski force-pushed the elastic-search-update branch 2 times, most recently from 37eb60b to 2f409ae Compare March 26, 2024 13:06
@mjankowski mjankowski force-pushed the elastic-search-update branch 2 times, most recently from 637aa03 to 9356a78 Compare March 27, 2024 18:55
@mjankowski
Copy link
Contributor Author

Possible interim path here -- looks like the ES gem in its newer 7.x releases, supports either faraday 1 or 2. If we can get Chewy (still in 7.x series) to relax it's requirement to allow newer ES7 gem, we could get a relaxed faraday req prior to ES8 update on the chewy side. Opened a PR to hopefully get discussion/approval of the idea: toptal/chewy#933

@mjankowski
Copy link
Contributor Author

This is rebased/updated, but blocked on both a) the chewy version 8 support/PR; b) the chewy 7 relaxed ES version support PR. Chewy appears to be only sort of maintained at this point ... not sure how to advance this short of more actively forking or relying on a PR branch there...

Copy link
Contributor

github-actions bot commented May 6, 2024

This pull request has merge conflicts that must be resolved before it can be merged.

@mjankowski
Copy link
Contributor Author

Rebased after #30157 merged.

This is still pointing to the WIP PR branch for chewy 8. I'll keep monitoring that and update if/when that releases.

Fortunately, we got the faraday range flexibility from the 7.x branch, so this is less pressing as a result.

Copy link
Contributor

github-actions bot commented May 6, 2024

This pull request has resolved merge conflicts and is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/search dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants