Skip to content

Conversation

@bernardocs
Copy link

Fixes #52
Revisiting PR #53
Did a rebase with master
I got a little confused over the tests though... I would probably welcome some advice on these. I'll try to include it later anyway 😃

@bernardocs
Copy link
Author

Also identified (here) that the new ElasticSearch client requires Node >8 ...
With that said, I just removed a little destructuring that I found...

I'll try to run a linter later

@bernardocs bernardocs requested a review from adrai February 3, 2020 21:50
@bernardocs
Copy link
Author

bernardocs commented Feb 3, 2020

The tests that worked on TravisCI fail locally
Should I try to include elasticsearch and @elasticsearch on this collection to see if things run smoothly on Travis? 🤔

@adrai
Copy link
Collaborator

adrai commented Feb 5, 2020

can you remove the package-lock.json file and not yet update the version in the package.json?

@adrai
Copy link
Collaborator

adrai commented Feb 5, 2020

The tests that worked on TravisCI fail locally
Should I try to include elasticsearch and @elasticsearch on this collection to see if things run smoothly on Travis? 🤔

you can try to include your new @elasticsearch in that collection

@bernardocs
Copy link
Author

bernardocs commented Feb 5, 2020

ElasticSearch new package (@elastic/elasticsearch) was not in the package.json (duh) and, even so, the client has changed a little the return body... that's why the tests went red...
I'll try to fix today

package.json Outdated
"lib": "./lib"
},
"dependencies": {
"@elastic/elasticsearch": "^7.5.1",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move this to devDependencies

@bernardocs
Copy link
Author

bernardocs commented Feb 5, 2020

I noticed that the Elasticsearch store implementation tries to use TTL as a mean to invalidate sessions (correct me if I'm wrong)... but there's a problem with that: TTL feature has been deprecated since version 5.x and removed altogether on version 7.x

The tests throw an error when trying to put session mapping with _ttl saying:

Root mapping definition has unsupported parameters: [_ttl : {default=14d, enabled=true}]

Should we review the support for ElasticSearch altogether? 👀

Refer to (...) for more info:
https://www.elastic.co/blog/ttl-documents-shield-and-found
https://discuss.elastic.co/t/why-es-remove-ttl-and-how-it-is-replaced/60449/2

@adrai
Copy link
Collaborator

adrai commented Feb 5, 2020

I'm not an elasticsearch expert... maybe we should ask @jackpilowsky @tony-cocco @ewjmulder ?

@jackpilowsky
Copy link

_timestamp and _ttl
The _timestamp and _ttl fields were deprecated and are now removed. As a replacement for _timestamp, you should populate a regular date field with the current timestamp on application side. For _ttl, you should either use time-based indices when applicable, or cron a delete-by-query with a range query on a timestamp field.

https://www.elastic.co/guide/en/elasticsearch/reference/5.0/breaking_50_mapping_changes.html

@jackpilowsky
Copy link

I will need this regardless in the long term, but my team is not ready to move to ES 7 yet and my backlog is looking pretty long. Might take a few weeks before I can create a workaround

@ewjmulder
Copy link
Contributor

@adrai I would have liked to help out but since that one fix PR in 2016 I've not worked much with elasticsearch, so I'm afraid I cannot be of any help, good luck!

@jackpilowsky
Copy link

@adrai , @bernardocs

Please check jackpilowsky@d3cba7c

I had to make significant changes to the way the library works. Some functions don't work with globbed index names (client.exists and client.get in particular).

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.

Support for new Elasticsearch Node.js Client

4 participants