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

Update ElasticSearch to 8.0 #3026

Closed
wants to merge 1 commit into from

Conversation

RC-Lee
Copy link
Contributor

@RC-Lee RC-Lee commented Feb 23, 2022

Issue This PR Addresses

#2913

Type of Change

  • Update: Updates ElasticSearch to 8.0 on Telescope

Description

  • Updating package.json ES dependency to 8.0
  • Updating docker/docker-compose.yml to ES 8.0
  • Disabling new ES default security settings
  • Refactor: Taking out 'type' from 'indexer' and 'search'
  • Refactor: Commenting out ES-mock test instances

Co-authored-by: @manekenpix

Steps to test the PR

To run on development

  1. pnpm install
    • To test legacy parser (current working parser)
docker-compose --env-file ./config/env.development up --build redis elasticsearch
  • To test Search
docker-compose --env-file ./config/env.development up --build redis elasticsearch posts traefik search nginx planet
  1. pnpm start. Sometimes if it can't find ElasticSearch, you'd have to stop it ( ctrl + c ) and then run pnpm start again.

Problems

  1. Currently for the new security features we have it set to false. Details of the conversation can be found in the issue at this comment

  2. The changes for createError in the Search query.js is related to issue [Satellite] Cannot set property statusCode of Error which has only a getter #2755 , which can be fixed by two different PRs, because this is an Error from ElasticSearch, to be fixed by Fixes #2755: Update ElasticSearch URL  #2990 and a crash from http-errors, to be fixed by Satelitte PR#64 (Yes, I'm also asking for reviews please).

The problem is that in development mode, ElasticSearch sometimes can't recognize the url 127.0.0.1 and tries to send an ElasticSearch Error, which causes the crash when we try to createError from it.

The current code changes in query.js can be taken off once at least one of these two fixes are implemented.

  1. ElasticSearch Mock does not work with ES 8.0
    I've tried to upgrade ES to 8.0 in Satellite and run the tests there as well. I get this error on the ES-mock tests:
Test suite failed to run
                                                                                                                                                                                 
    TypeError: Class extends value undefined is not a constructor or null

       9 |   const client = new Client({
      10 |     node: 'http://localhost:9200',
    > 11 |     Connection: mock.getConnection(),
         |                      ^
      12 |   });
      13 |   // Provide a fake health check
      14 |   client.cluster.health = () => Promise.resolve();

      at buildConnectionClass (node_modules/.pnpm/@[email protected]/node_modules/@elastic/elasticsearch-mock/index.js:122:32)
      at Mocker.getConnection (node_modules/.pnpm/@[email protected]/node_modules/@elastic/elasticsearch-mock/index.js:117:12)
      at new MockClient (src/elastic.js:11:22)
      at Elastic (src/elastic.js:32:14)
      at test.js:1014:20
      at test.js:1013:3

An issue for ES-mock on this was made a while ago. I don't think this is fixable from our end.
So, unfortunately, I had to temporarily comment out the Search tests that required ES-mock to run.
We'd have to either wait for ES-Mock to update, or find a new way to test Search. Maybe mock the whole module.

  1. Search currently runs ES v7.17 from Satellite, we've yet to test it out on ES 8.0

- Updating package.json ES dependency to 8.0
- Updating docker/docker-compose.yml to ES 8.0
- Disabling new ES default security settings
- Refactor: Taking out 'type' from 'indexer' and 'search'
- Refactor: Commenting out ES-mock test instances

Co-authored-by: Josue <[email protected]>
@gitpod-io
Copy link

gitpod-io bot commented Feb 23, 2022

@RC-Lee RC-Lee requested a review from manekenpix February 23, 2022 08:10
@RC-Lee RC-Lee self-assigned this Feb 23, 2022
@aserputov aserputov added this to the 2.8 Release milestone Feb 23, 2022
@aserputov aserputov added dependencies Pull requests that update a dependency file review: normal labels Feb 23, 2022
@humphd
Copy link
Contributor

humphd commented Feb 23, 2022

Based on what you've written above, it seems like it's too soon to move to 8.0, no? If we don't have mocks, we don't have tests. Seems like a deal breaker?

@cindyorangis
Copy link
Contributor

cindyorangis commented Feb 26, 2022

I think we should wait until elasticsearch-mock is compatible with Elasticsearch v8 before we update Elasticsearch. I don't want to go in naked with no mocks or tests.

@humphd
Copy link
Contributor

humphd commented Feb 26, 2022

Maybe @rclee91 will send a PR upstream to fix it, who knows...

@tpmai22 tpmai22 modified the milestones: 2.8 Release, 2.9 Release Mar 8, 2022
@humphd
Copy link
Contributor

humphd commented Mar 11, 2022

elastic/elasticsearch-js-mock#24 just landed, updating the mock to support v8.

@RC-Lee
Copy link
Contributor Author

RC-Lee commented Mar 11, 2022

I've started to test this with Satellite.
Currently I'm having trouble getting it to get the right ES and ES-mock versions for the Satellite within Telescope
But I have managed to get the updates and tests working with the old Satellite (in its own repo)
I'll continue to tinker around a bit.

@AmasiaNalbandian
Copy link
Contributor

Hello! Please rebase, as the following commit 6046273 has reorganized our file structure!

@nguyenhung15913
Copy link
Contributor

@rclee91 Hi, can you give me an update for this. Do you need help or more reviewers?

@RC-Lee
Copy link
Contributor Author

RC-Lee commented Mar 31, 2022

closing until after autocomplete has been fully implemented.

@RC-Lee RC-Lee closed this Mar 31, 2022
@RC-Lee RC-Lee deleted the issue#2913-update-ES-8.0 branch October 6, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants