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

Add a basic deprecation warning that the JSON format is changing in v9 #114739

Open
wants to merge 13 commits into
base: 8.x
Choose a base branch
from

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Oct 14, 2024

Warn that #90529 changes the json format when detailed errors are turned off

@thecoop thecoop added >non-issue :Core/Infra/REST API REST infrastructure and utilities v8.16.0 labels Oct 14, 2024
@thecoop thecoop requested a review from rjernst October 14, 2024 15:48
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Oct 14, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@thecoop
Copy link
Member Author

thecoop commented Oct 14, 2024

I'm not sure if this needs to be a critical (as things are definitely changing), or a warning, as the user can't do anything about it until they upgrade.

Do we need to include a link to more detailed docs (which don't really exist, apart from the breaking changelog?)

@DaveCTurner
Copy link
Contributor

the user can't do anything about it until they upgrade.

That seems bad to me. Certainly this can't be critical because critical means "must be addressed before you upgrade". But could we have a way for users to opt-in to the new format ahead of time? Should we fall back to the old format in v9 if the user requests RestApiVersion.V_8?

@thecoop
Copy link
Member Author

thecoop commented Oct 17, 2024

This is the behaviour as agreed in the BwC discussion...

@DaveCTurner
Copy link
Contributor

Ok I see, I wasn't in that discussion 😄

Can we tell folks to opt-in to the future behaviour now by removing http.detailed_errors.enabled from their node settings?

@thecoop
Copy link
Member Author

thecoop commented Oct 17, 2024

That will cause detailed error information to be output, which is probably not what they want (as they set the option in the first place)

@DaveCTurner
Copy link
Contributor

DaveCTurner commented Oct 17, 2024

Sure but we've got to do something here, we can't just expect folks to upgrade and suck it up if their clients can't handle the new format caused by the upgrade. Either we need to help them fix their clients before upgrading or else we need to preserve the v8 error format in v9.

@thecoop
Copy link
Member Author

thecoop commented Oct 17, 2024

That's what the decision of the BwC committee was - there's very little use of this option, so any attempt at pro-active mitigation is disproportionate. You can re-open the discussion with BwC if you want?

@DaveCTurner
Copy link
Contributor

That's what the decision of the BwC committee was

That's not my reading of the decision. There was no mention of this change affecting clients that requested v8 compatibility in v9, and our default position for that case would be that such clients see no change in behaviour. Would you ask the BwC committee to clarify whether they intended to make this change in v8-compat mode too?

@rjernst
Copy link
Member

rjernst commented Oct 17, 2024

I agree with Dave's concern. The point of rest api compatibility is to bridge these exact gaps, changes across majors. I think we should maintain the existing format when the v8 header is used.

@thecoop
Copy link
Member Author

thecoop commented Oct 18, 2024

I've added BwC API outputs to #90529, and tweaked the warning here accordingly

Comment on lines +72 to +73
"The JSON format of non-detailed errors will change in Elasticsearch 9.0 to match the JSON structure"
+ " used for detailed errors. To keep using the existing format, use the V8 REST API."
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the same deprecation warning in the v9 code too (when hitting this path in v8 compat mode).

I also don't think we can have a critical warning without any way for folks to resolve it before upgrading.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed it to a warning, and will copy these changes to the v9 PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >non-issue Team:Core/Infra Meta label for core/infra team v8.17.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants