Skip to content

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Oct 14, 2024

Backport #116330 to 8.17

@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

@thecoop thecoop force-pushed the exception-json-warning branch from 1467b4c to a00cc1d Compare November 6, 2024 13:26
@thecoop thecoop requested review from a team and DaveCTurner November 6, 2024 13:28
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM except that I'd rather we followed the usual process of merging this into main, backporting it to 8.x, and then merging the remnants of #90529 into main. As it is, it's overly hard to review the difference between this PR and #90529.

@thecoop
Copy link
Member Author

thecoop commented Nov 6, 2024

Hmm, ok, yes we could do it that way

@thecoop
Copy link
Member Author

thecoop commented Nov 6, 2024

See #116330

@thecoop thecoop changed the title Add a basic deprecation warning that the JSON format is changing in v9 Add a basic deprecation warning that the JSON format is changing in v9 (#116330) Nov 6, 2024
@thecoop thecoop changed the title Add a basic deprecation warning that the JSON format is changing in v9 (#116330) [8.x] Add a basic deprecation warning that the JSON format is changing in v9 (#116330) Nov 6, 2024
@thecoop
Copy link
Member Author

thecoop commented Nov 6, 2024

@elasticmachine update branch

@thecoop
Copy link
Member Author

thecoop commented Nov 12, 2024

@elasticmachine update branch

@thecoop thecoop force-pushed the exception-json-warning branch 2 times, most recently from 5c4f41c to 82f23e1 Compare November 13, 2024 10:23
Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

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

LGTM, just a small suggestion.

@thecoop thecoop merged commit c441ada into elastic:8.x Nov 13, 2024
4 of 15 checks passed
@thecoop thecoop deleted the exception-json-warning branch November 13, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport :Core/Infra/REST API REST infrastructure and utilities >deprecation 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.

7 participants