Default node filter does not seem filter for master only nodes

[spinscale]

🐛 Bug Report

The implementation has a default node filter of return true, as seen here https://github.com/elastic/elastic-transport-js/blob/main/src/Transport.ts#L770-L772

The docs state that the default filter however exclude master-only nodes, see https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/basic-config.html

I think the documentation states the correct behaviour and the actual implementation needs to be adapted - unless I am looking at the wrong part of the code.

[JoshMock]

Looks like that was an oversight during a large rewrite a couple years back. I'll put the old implementation back in soon and let you know here when it's shipped.

[JoshMock]

Copying over from the discuss forum post:

On closer investigation, it seems roles was a vestigial value from 7.x that partly made its way into 8.x, but is not available when it would actually be useful. You can see that roles is implicitly stripped out right here by omission, before passing the node options on to the transport as ConnectionOptions, which does not support roles, or any other custom metadata.

So, it appears that the better choice would actually be to drop roles from the NodeOptions type and update the documentation so that it's clear that nodeFilter is passed Connection values, not NodeOptions, and always returns true by default.

Given that, it's not obvious what nodeFilter should do by default other than what it already does. Without roles that are explicitly assigned at instantiation, we don't know enough about a node to make any useful choices.

If you have a particular use case in mind for how to use nodeFilter, I'd recommend creating your own custom class that extends Connection with the addition of metadata about your nodes that would appear at the right time.

Let me know if you have more questions! For now I'm going to update the docs to reflect the way the code currently works.

[JoshMock]

We discussed further and decided to reintroduce the functionality rather than stripping out the broken feature. Will handle that in the next few days.

[JoshMock]

This is fixed in @elastic/transport 9.0.1 and 8.9.6, which just published.

The fix to reinstate it into @elastic/elasticsearch 8.x will go out in 8.18.1 shortly.

[JoshMock]

Fix is now available in 8.18.1.