Skip to content

Perform precise check for types warnings in cluster restart tests. #37944

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 13, 2019
Merged

Perform precise check for types warnings in cluster restart tests. #37944

merged 3 commits into from
Feb 13, 2019

Conversation

@jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jan 28, 2019 •
edited
Loading

Instead of using WarningsHandler.PERMISSIVE, we only match warnings
that are due to types removal.

This PR also renames allowTypeRemovalWarnings to allowTypesRemovalWarnings.

Relates to #37920.

@jtibshirani jtibshirani added >test Issues or PRs that are addressing/adding tests :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 labels Jan 28, 2019
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 28, 2019

Pinging @elastic/es-search

@jtibshirani jtibshirani force-pushed the warnings-in-cluster-restart-tests branch 2 times, most recently from fa3cc17 to 6d76588 Compare January 29, 2019 21:32
@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jan 29, 2019

@elasticmachine run elasticsearch-ci/2

markharwood
markharwood reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@markharwood markharwood left a comment

Choose a reason for hiding this comment

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

I left a comment re warning message and build looks to have failed but otherwise +1 for making the warnings checks less permissive.

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestCreateIndexAction.java Outdated
Copy link
Contributor

@markharwood markharwood Jan 30, 2019

Choose a reason for hiding this comment

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

In a separate PR I was reconsidering this warning message. The deprecation part is clear enough but the solution only mentions setting a flag "to be compatible with the next major version". Maybe we should be more explicit stating that the user needs to both:

  1. Remove the type from the usual mapping -> type -> properties hierarchy and
  2. Tell us they've done this with the include_type_name=false param

It's unfortunate that doing step 1 requires users to also do 2 but that's where we are.

Copy link
Contributor Author

@jtibshirani jtibshirani Jan 30, 2019

Choose a reason for hiding this comment

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

That does sound clearer to me! Would you be able to make that update on 6.x? Then I could hold this PR until that merges, and update the message here before merging.

Copy link
Contributor Author

@jtibshirani jtibshirani Jan 30, 2019

Choose a reason for hiding this comment

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

An update: I ended up opening #38052 to clarify the messages, it would be great if you could review it.

@jtibshirani jtibshirani mentioned this pull request Jan 30, 2019
Merged
@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jan 31, 2019

@elasticmachine run elasticsearch-ci/2

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Feb 1, 2019

@markharwood with the new deprecation message, does this now look good to you?

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Feb 1, 2019

After catching up offline, we decided to change the check to verify the message starts with [types removal], as opposed to matching the exact message from 6.7.

@markharwood
Copy link
Contributor

markharwood commented Feb 4, 2019

I just created #38310 which adds a new "EsRestTestCase.expectTypesRemovalWarnings" method. As we discussed this just checks for any warning messages starting with [types removal] rather than having master rely on a copy of the exact phrasing of messages used in 6.x branch (as they could easily deviate).

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Feb 4, 2019 •
edited
Loading

@markharwood I made an update to use the method allowTypesRemovalWarnings from your PR. This should be ready for another look.

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019
@jtibshirani
Perform precise types warnings check in cluster restart tests.
c461b22 
Instead of using `WarningsHandler.PERMISSIVE`, we have preferred to match
on the specific warning message.
@jtibshirani
Update to the latest deprecation message for 6.7.
1d5ec91
@jtibshirani jtibshirani force-pushed the warnings-in-cluster-restart-tests branch from 14c8bbb to d0ca156 Compare February 12, 2019 00:37
@jtibshirani jtibshirani force-pushed the warnings-in-cluster-restart-tests branch from d0ca156 to 26c5a4d Compare February 12, 2019 01:03
markharwood
markharwood approved these changes Feb 13, 2019
View reviewed changes
Copy link
Contributor

@markharwood markharwood left a comment

Choose a reason for hiding this comment

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

LGTM

@jtibshirani jtibshirani merged commit acb8b43 into elastic:master Feb 13, 2019
@jtibshirani jtibshirani deleted the warnings-in-cluster-restart-tests branch February 13, 2019 18:40
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 13, 2019
@jasontedor
Merge branch 'master' into retention-lease-ccr
e58bbbc 
* master:
  Fix excessive increments in soft delete policy (elastic#38813)
  Perform precise check for types warnings in cluster restart tests. (elastic#37944)
  [ML] Extract base class for integ tests with native processes (elastic#38850)
  Add get file chunk timeouts with listener timeouts (elastic#38758)
  Fix PreConfiguredTokenFilters getSynonymFilter() implementations (elastic#38839)
jtibshirani added a commit that referenced this pull request Feb 13, 2019
@jtibshirani
Perform precise check for types warnings in cluster restart tests. (#…
e769cb4 
…37944)

Instead of using `WarningsHandler.PERMISSIVE`, we only match warnings
that are due to types removal.

This PR also renames `allowTypeRemovalWarnings` to `allowTypesRemovalWarnings`.

Relates to #37920.
jtibshirani added a commit that referenced this pull request Feb 13, 2019
@jtibshirani
Perform precise check for types warnings in cluster restart tests. (#…
d35a049 
…37944)

Instead of using `WarningsHandler.PERMISSIVE`, we only match warnings
that are due to types removal.

This PR also renames `allowTypeRemovalWarnings` to `allowTypesRemovalWarnings`.

Relates to #37920.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@markharwood markharwood markharwood approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Search Foundations/Mapping Index mappings, including merging and defining field types >test Issues or PRs that are addressing/adding tests v7.0.0-rc1 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jtibshirani @elasticmachine @markharwood @jakelandis @jasontedor @michaelbaamonde