Update to work with elasticsearch 8 (maintains support for elasticsearch 7) #488
Conversation
Based on the existence of pelias/pelias#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. |
|
Hi @michaelkirk, 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. |
|
Hey @michaelkirk 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
force-pushed
the
mkirk/elastic8
branch
from
8ac201c
to
f443739
Compare
🙏 Thank you!
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.
🙏 Thank you!
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 |
michaelkirk
force-pushed
the
mkirk/elastic8
branch
4 times, most recently
from
45cd958
to
f086bb5
Compare
michaelkirk
force-pushed
the
mkirk/elastic8
branch
2 times, most recently
from
5402f06
to
e698faa
Compare
michaelkirk
force-pushed
the
mkirk/elastic8
branch
2 times, most recently
from
50c0cd1
to
b3109f3
Compare
elastic7 seems to start up faster than 8, or maybe it was just random jitter, but in any case we probably want to avoid spurious timeouts like this. e.g. see elastic7 which started up: ://github.com/michaelkirk-pelias/schema/actions/runs/5272730400/jobs/9535345934 vs. elastic8 which timed out: https://github.com/michaelkirk-pelias/schema/actions/runs/5272730400/jobs/9535346043
With xpack security, elastic8 starts with TLS and user/password auth, which is unnecessary configuration for CI.
The previous package was producing an error[^1]. Given that the old npm
package is deprecated[^2], I've switched to the new recommended client.
The new npm package has a peculiar (though well reasoned) versioning
scheme.
The major version of the new npm package denotes the minimum supported
elasticsearch server version.
The package is (apparently) given forward compatibility with newer
version. So in this case, using version 7, means we can talk to
elasticsearch7 and elasticsearch8 (but not elasticsearch6).
This does mean that breaking changes, like dropping node versions, can
occur in a minor release, hence the more restrictive versioning
constraint (~7.17.0 instead of ^7.17.0).
[1] fixes error:
```
--------------
create index
--------------
/home/runner/work/schema/schema/node_modules/elasticsearch/src/lib/utils.js:303
throw new TypeError(err);
^
TypeError: Invalid apiVersion "8.0", expected a function or one of
_default, 7.6, 7.5, 7.4, 7.3, 7.2, 7.1, 7.0, 6.8, 5.6, 7.7, 7.x, master
at Object.utils.funcEnum
(/home/runner/work/schema/schema/node_modules/elasticsearch/src/lib/utils.js:303:13)
at new Client
(/home/runner/work/schema/schema/node_modules/elasticsearch/src/lib/client.js:74:33)
at Object.<anonymous>
(/home/runner/work/schema/schema/scripts/create_index.js:11:16)
at Module._compile (node:internal/modules/cjs/loader:1196:14)
at Object.Module._extensions..js
(node:internal/modules/cjs/loader:1250:10)
at Module.load (node:internal/modules/cjs/loader:1074:32)
at Function.Module._load (node:internal/modules/cjs/loader:909:12)
at Function.executeUserEntryPoint [as runMain]
(node:internal/modules/run_main:81:12)
at node:internal/main/run_main_module:22:47
Error: Process completed with exit code 1.
```
[2] old package is deprecated
From https://www.npmjs.com/package/elasticsearch
> ⚠️ This client is no longer maintained. We strongly advise you to
migrate to the [new Elasticsearch
client](https://www.elastic.co/blog/new-elasticsearch-javascript-client-released).
- response body moved from `res` to `res.body` - nodes.info takes *no* arg rather than null before callback - config.hosts became config.nodes (and changed to string format)
We can just use the default here.
- On success, elasticsearch errors are now *null* rather than *undefined*,
which broke some tests. Now we just check that `err` is falsy rather than strictly
equal to `undefined`.
- Payload moved from res to res.body
- `statusCode` moved from top level callback arg to `res.statuCode`
- normalized function formatting for tests to `(err, { body })` there
were a few different styles/indentations.
The legacy elasticsearch client had an integrated stdout logger. The new elasticsearch client does not, so instead we use the pelias-elasticsearch which integrates the modern elasticsearch client with pelias-logger.
michaelkirk
force-pushed
the
mkirk/elastic8
branch
from
b3109f3
to
811b45f
Compare
|
I've rebased this - it's still a draft though because it depends on the new config format in pelias/config#141 |
|
In general, I would like to hold off making the switch from the deprecated 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 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 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. |
|
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'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. |
I'm sorry my first PR is so large.
Here's the reason for this change 🚀
I'd like to keep pelias compatible with the latest elasticsearch and replace some unmaintained libraries, so I've switched the deprecated
elasticsearchclient out for it's successor@elastic/elasticsearchand 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: pelias/pelias#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 👏
elasticsearchpackage for it's successor@elastic/elasticHere's how others can test the changes 👀
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-metroexample 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.