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

Improve the deprecation messages around include_type_name. #38052

Merged
merged 2 commits into from
Jan 31, 2019

Conversation

jtibshirani
Copy link
Contributor

This PR standardizes the message across the different indices APIs and makes
sure to mention that mappings should no longer be nested under the type name.

Thanks @markharwood for this feedback in #37944 (comment).

This PR standardizes the message across the different indices APIs and makes
sure to mention that mappings should no longer be nested under the type name.
@jtibshirani jtibshirani added >non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0 labels Jan 30, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

"requests is deprecated. The parameter include_type_name should be provided and set to false to be " +
"compatible with the next major version.";
"requests is deprecated. To be compatible with 7.0, the mapping definition should not nested under the " +
"type name, and the parameter include_type_name must be provided and set to false.";
Copy link
Contributor

Choose a reason for hiding this comment

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

"should not nested"

Worrying, this decline in the be population.

Copy link
Contributor

Choose a reason for hiding this comment

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

"the mapping definition should not nested under the type name"

Thanks Julie, I am ok with this message as well, but may be more explicit instruction what needs to be done could be more clear, e.g using Mark's comment "To be compatible with 7.0, the type must be removed from the usual mapping -> type -> properties hierarchy, and the parameter ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worrying, this decline in the be population.

Indeed!

"compatible with the next major version.";
public static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Specifying types in create index " +
"requests is deprecated. To be compatible with 7.0, the mapping definition should not nested under the " +
"type name, and the parameter include_type_name must be provided and set to false.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Another missing "be".

@markharwood
Copy link
Contributor

markharwood commented Jan 31, 2019

Do you think it would be overkill to make a constant for parts of this message?

It may come in handy if we decide to change the message with with another idea I had - at some pointing adding a url e.g. "see http://elastic.co/migrating_types.html for more". We could have JSON examples of the old and new formats there. I found such a warning message pretty useful the other day. I was running an npm install which warned of potential vulnerabilities in code being installed and it included a URL to a page giving useful details behind the particular vulnerability. Way more info than you can pack into a warning message.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

Thanks Julie!

"requests is deprecated. The parameter include_type_name should be provided and set to false to be " +
"compatible with the next major version.";
"requests is deprecated. To be compatible with 7.0, the mapping definition should not nested under the " +
"type name, and the parameter include_type_name must be provided and set to false.";
Copy link
Contributor

Choose a reason for hiding this comment

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

"the mapping definition should not nested under the type name"

Thanks Julie, I am ok with this message as well, but may be more explicit instruction what needs to be done could be more clear, e.g using Mark's comment "To be compatible with 7.0, the type must be removed from the usual mapping -> type -> properties hierarchy, and the parameter ..."

" mapping requests is deprecated. The parameter will be removed in the next major version.";

public static final String TYPES_DEPRECATION_MESSAGE = "[types removal] The response format of get index " +
"template requests will change in 7.0. Please start using the include_type_name parameter set to false to " +
Copy link
Contributor

Choose a reason for hiding this comment

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

index template requests

Shouldn't it be "get mapping requests"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@jtibshirani
Copy link
Contributor Author

Thank you both for the review and for catching my typos, I must have been staring at these deprecation messages for too long!

Do you think it would be overkill to make a constant for parts of this message?

I didn't see a huge benefit to doing so in this PR, since there are a small number of uses of each message and it would cause us to diverge from 7.0 more. However, we've discussed doing a better job unifying the messages in general (and maybe pulling them out of the Rest*Action classes), so maybe this would be something to look into on master (with a backport)?

It may come in handy if we decide to change the message with with another idea I had - at some pointing adding a url e.g. "see http://elastic.co/migrating_types.html for more". We could have JSON examples of the old and new formats there.

Just a note that in #38003 we've added high-level examples of the changes users will have to make in 7.0 to the existing 'removal of types' page. Pulling these examples out into their own page like 'migrating types' and expanding on them could be really nice! If we ended up updating the deprecation warnings to refer to the page, I think we would want to do that in a separate PR against master, since it is not just these messages here that would need updating.

the type must be removed from the usual mapping -> type -> properties hierarchy

This is a helpful description, but I didn't think it was quite precise because you don't always have properties in mappings definitions (and in fact the definition could be empty). If it's okay with you, I'll plan to keep this message with the understanding that the user can look at the API documentation if they're confused, or refer to the 'removal of types' page.

@jtibshirani jtibshirani merged commit 4267b8f into elastic:6.x Jan 31, 2019
@jtibshirani jtibshirani deleted the types-deprecation-message branch January 31, 2019 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants