search

pelias / schema

elasticsearch schema files and tooling
MIT License
40 stars 76 forks source link

Update to work with elasticsearch 8 (maintains support for elasticsearch 7) #488

Open michaelkirk opened 1 year ago

michaelkirk commented 1 year ago

I'm sorry my first PR is so large.

Here's the reason for this change :rocket:

I'd like to keep pelias compatible with the latest elasticsearch and replace some unmaintained libraries, so I've switched the deprecated elasticsearch client out for it's successor @elastic/elasticsearch and made the necessary changes to get CI passing with es8.

Based on the pre-existing const SUPPORTED_ES_VERSIONS = '>=7.4.2', I infer it's OK to make changes that might not work on ES version 6 or lower, but let me know if that's incorrect and I can try again.

Note that this is a draft PR because it depends on a couple of forked dependencies:

Note also that this is only one piece of the puzzle to getting the full pelias project on es8, but it's a start! I'm planning to work on the other pieces too. Let me know if you have any advice or thoughts. edit: es8 tracking issue now exists here: https://github.com/pelias/pelias/issues/953

The individual commits are intended to be well factored, so the descriptions might be illuminating if you are confused about a particular change.

Here's what actually got changed :clap:

Here's how others can test the changes :eyes:

I added a couple of new builds to CI for es-8.0.0 and the most recent 8.8.0. I've tested it on the portland-metro example project, but it would be good to test it on others.

Also, though I've administered a pelias instance for Headway for 6months or so, I'm new to the code base, so let me know if I should do something differently.

michaelkirk commented 1 year ago

I infer it's OK to make changes that might not work on ES version 6 or lower, but let me know if that's incorrect and I can try again.

Based on the existence of https://github.com/pelias/pelias/issues/881, this may have been an overzealous assumption. It seems like creating on ES6 has not been supported for a while - but does the latest pelias release still support reading from es6?

It might be reasonable to break es6 compatibility at this point, but I would only want to do so intentionally.

orangejulius commented 1 year ago

Hi @michaelkirk, Thanks for all the work here.

It's definitely safe to completely drop ES6 support at this point. Generally it's only feasible to support any two adjacent versions of Elasticsearch. Our past upgrades have followed this pattern, where in order to add support for the newest version we have to first drop support for the release line 2 major versions behind.

We should be able to support both ES7 and ES8 for quite some time, until we presumably add support for ES9 after first dropping ES7.

orangejulius commented 1 year ago

Hey @michaelkirk I'm back with some more feedback on this PR.

I just updated the "official" place where we document Elasticsearch version support so it no longer includes Elasticsearch 6. In practice, we haven't taken steps to preserve Elasticsearch 6 support in a long time, but at least now it's for real.

As a first step, would you want to pick out some of the changes from this PR that only drop things no longer supported in ES7? Think of it as a way to split up this big PR into things we could unambiguously merge immediately. Then we can work on the harder stuff piece by piece.

Good things to include would be anything related to types (no longer needed at all), deprecation fixes, etc.

Things that we should talk about separately and would be good for subsequent PRs are:

michaelkirk commented 1 year ago

I just updated the "official" place where we document Elasticsearch version support so it no longer includes Elasticsearch 6. In practice, we haven't taken steps to preserve Elasticsearch 6 support in a long time, but at least now it's for real.

🙏 Thank you!

As a first step, would you want to pick out some of the changes from this PR that only drop things no longer supported in ES7? Think of it as a way to split up this big PR into things we could unambiguously merge immediately. Then we can work on the harder stuff piece by piece.

Yes, I can split up this big PR. I'm personally working on getting some of the examples to run to make sure we're headed in the right direction, but once some of that is working, I'll try to cleave off and rebase functionality to make it easier to review.

Node.js upgrades in CI (handled through https://github.com/pelias/pelias/issues/950)

🙏 Thank you!

Moving to the new Elasticsearch NPM module (I honestly would love to find out if we could avoid this completely, I personally don't love how they manage different ES versions in it, but that may not be possible)

Do you mean how they use their javascript module version to denote which version of elasticsearch is supported? I agree it's uncommon. Regardless, it is the maintained and official way to talk to elasticsearch from javascript. The old module hasn't been tended to in over a year and doesn't support es8. So, I'm thinking @elastic/elasticsearch is the way to go unless you want to delve into permanent fork and upkeep territory.

michaelkirk commented 8 months ago

I've rebased this - it's still a draft though because it depends on the new config format in https://github.com/pelias/config/pull/141

missinglink commented 8 months ago

In general, I would like to hold off making the switch from the deprecated elasticsearch npm module to the newer @elastic/elasticsearch one until it can be better tested.

This repo/branch serves as a great place to begin testing as it's a 'one-off' job of inserting the schema into elasticsearch, and as such doesn't have to worry about things like error handling under load which could cause downtime in a repo such as pelias/api.

As of yesterday the current state of Pelias is that es6 support is dropped, es7 remains the recommended version, es8 is passing schema integration tests and so is theoretically supported.

In order to continue using the legacy elasticsearch npm module under es8 we need to keep the pelias.json setting esclient.apiVersion=7.x. Elastic provide a single version of API backwards compatibility.

I've also published a 8.12.2-beta docker image which can be used for testing.

Once es9 is released we will have no choice but to upgrade the npm module, so we will revisit this PR, at the latest, at that time.

I would encourage people to start testing es8 and let us know how it goes, it would be nice to have some bugs caught and fixed before we roll it out into production environments.

michaelkirk commented 8 months ago

It somehow didn't occur to me that we could make the jump to elastic8 without jumping to the modern client. Great observation @missinglink.

I would like to hold off making the switch from the deprecated elasticsearch npm module to the newer @elastic/elasticsearch one until it can be better tested.

I'd still like to see pelias get on the supported client, but now I realize we don't even necessarily have to do that across all repositories at once.

My plan is to queue up a new planet build for https://maps.earth using all your recently published containers (including the elasticsearch8 build). I'll let you know if I encounter any issues, though we don't get a ton of traffic.

Let me know if there's anything else I can do to help test.