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

Enable HLRC compatibility mode by default #86517

Merged
merged 8 commits into from
May 12, 2022
Merged

Conversation

swallez
Copy link
Member

@swallez swallez commented May 6, 2022

HLRC compatibility mode allows it to communicate with Elasticsearch 8.x. Users are generally unaware of this feature, and this leads to a lot of questions and issues about the migration from 7.x to 8.x and the transition from HLRC to the new Java API client (we recently added docs there as well).

This PR enables compatibility mode by default so that HLRC will work out of the box with versions 7.x and 8.x without any additional configuration.

It also adds docs about it, with a prominent note on the index page linking to compatibility mode explanations.

Reviewers:

@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@elasticsearchmachine
Copy link
Collaborator

@swallez please enable the option "Allow edits and access to secrets by maintainers" on your PR. For more information, see the documentation.

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label May 6, 2022
@elasticsearchmachine
Copy link
Collaborator

Hi @swallez, I've created a changelog YAML for you.

@sethmlarson
Copy link
Contributor

Enabling compatibility headers by default will cause errors when HLRC connects to older server versions (pre-7.14 iirc?)

@swallez
Copy link
Member Author

swallez commented May 6, 2022

@sethmlarson indeed, and I've written this in the docs. I wrote 7.16 for good measure as this is when compat mode was added to HLRC, although it's likely to have been available in ES in a previous version.

This change is also consistent with the client compatibility policy that recommends to have client version <= server version.

Key point is that it will remove a lot of friction in the transition from ES 7 to ES 8 where a number of users think they are required to migrate to the Java API client, even if we have clear docs about it.

@sethmlarson
Copy link
Contributor

Makes sense! Glad there's an existing policy of server version being greater or equal to client version for HLRC we can lean on here.

@dakrone
Copy link
Member

dakrone commented May 6, 2022

This feels like a breaking change to me? This means someone going from a 7.17.3 to 7.17.4 version of the client might no longer be able to communicate with older versions of Elasticsearch. I'm not sure that I am comfortable making this change in a patch release.

Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

Thanks for these changes, docs LGTM!

@jakelandis
Copy link
Contributor

This means someone going from a 7.17.3 to 7.17.4 version of the client might no longer be able to communicate with older versions of Elasticsearch

This is a pretty big concern, and the reason we did not default in any of the 7.x clients. Is there specific reason why should change course, but only for the Java HRLC ? I will have to look up the exact version, but early 7.x clusters will fail all requests when presented with the compatibility header(s).

@swallez
Copy link
Member Author

swallez commented May 10, 2022

@dakrone @jakelandis I understand your points, but don't consider them as a concern. This change is consistent with the HLRC compatibility guidelines: "The High Level Client is guaranteed to be able to communicate with any Elasticsearch node running on the same major version and greater or equal minor version". Digging in the history, support for vnd.elasticsearch media types was added in 7.11 (see #63071 and #65362)

The goal is to remove a number of frustration points when migrating from 7 to 8. We've seen a number of reports where users try HLRC 7.x against ES 8.x, see failures as they did not enable compatibility mode, and end up considering that migration to the Java API client is a requirement. They not only feel we forced that migration on them, but feel blocked when encountering issues or missing features in the Java API client.

We've put a lot of engineering effort in making 8.x compatible with 7.x, but this feature is hidden so well that users don't know about it. Better docs is one side of the equation (see also the Java API client docs), provided that users actually read them. Having something that works out of the box provides the best experience, as it means users do not even have to know about it!

Regarding the change in a patch version, we have to consider that there will be no 7.18 so it's the only way for us to remove this roadblock.

The number of users running HLRC 7.17 against ES 7.10 or lower who may be impacted by this change is likely to be low and will fade over time, while migration from 7 to 8 is increasing and is the way forward. I'd rather explain users running HLRC 7.17.4 against ES 7.10 to disable compatibility mode or upgrade their cluster rather than deal with the frustration of users migrating from 7 to 8.

@dakrone
Copy link
Member

dakrone commented May 10, 2022

@swallez okay, since we document it that way, I think that it is reasonable to do this then. As a compromise, what do you think about marking this PR as >breaking so that it becomes more prevalent in the release documentation?

@elasticsearchmachine
Copy link
Collaborator

Hi @swallez, I've updated the changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

@swallez
Copy link
Member Author

swallez commented May 11, 2022

Thanks @dakrone. It indeed totally makes sense to document this as a breaking change. I've updated the release notes and fixed the minimal compatible version (7.11) in the docs in f9b546b

@swallez
Copy link
Member Author

swallez commented May 12, 2022

@dakrone I've updated the release notes and fixed a BWC test. Can you do a quick review?

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM

@swallez swallez merged commit 0cb5108 into 7.17 May 12, 2022
@swallez swallez deleted the hrlc-compat-on-by-default branch May 12, 2022 17:00
sethmlarson added a commit that referenced this pull request May 17, 2022
sethmlarson added a commit that referenced this pull request May 17, 2022
sethmlarson added a commit that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Clients Meta label for clients team v7.17.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants