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.

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

rm index: false from binary mappings #71343

Merged
merged 9 commits into from
Jul 13, 2020
Merged

rm index: false from binary mappings #71343

merged 9 commits into from
Jul 13, 2020

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Jul 9, 2020

Removes index: false from binary mappings. This is failing on snapshot verification builds -

 [mapper_parsing_exception] unknown parameter [index] on mapper [api_key] of type [binary] :: {"path":"/.kibana_1","query":{},"body":"{\"mappin.......

https://kibana-ci.elastic.co/job/elasticsearch+snapshots+verify/1056/execution/node/390/log/

@romseygeek / @nik9000 - we have this passing on an older snapshot and I'm wondering if elastic/elasticsearch@219b7db is related. Can you verify this option shouldn't be used?

@jbudz jbudz added review v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Jul 9, 2020
@jbudz jbudz requested review from jen-huang and a team July 9, 2020 22:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@tylersmalley
Copy link
Contributor

I wonder if this was an accidental breaking change. I wouldn't expect a breaking change like this to be introduced in 7.9.

@tylersmalley
Copy link
Contributor

Additionally, index: false was only recently added and is something which we want to keep for these binary types.

@jen-huang
Copy link
Contributor

jen-huang commented Jul 9, 2020 •
edited
Loading

@tylersmalley Docs say binary types are not stored and not searchable by default, doesn't that mean we don't need index: false on them? We added it initially a little while ago to help reduce Kibana saved object field limit, but it seems like ES changed since then.

@tylersmalley
Copy link
Contributor

@jen-huang, sounds like that would be the case. If so, should be OK to remove.

@jbudz have you verified this resolves the tests using the nightly? Might be worth updating the Jenkinsfile to verify: https://www.elastic.co/guide/en/kibana/current/development-es-snapshots.html#_for_pull_requests

@tylersmalley
Copy link
Contributor

tylersmalley commented Jul 10, 2020 •
edited
Loading

Type error was caused upstream.

@elasticmachine merge upstream

@romseygeek
Copy link

I wonder if this was an accidental breaking change. I wouldn't expect a breaking change like this to be introduced in 7.9.

Yes, this was accidental - apologies for the noise. I've created elastic/elasticsearch#59359 and will fix asap next week.

@romseygeek
Copy link

Adding index:false to an index created in 7x should now just yield a deprecation warning; in master you'll get an error (see elastic/elasticsearch#59381).

Thanks for identifying this so quickly. Stack FTW...

@EricDavisX EricDavisX mentioned this pull request Jul 13, 2020
Closed
@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020

@elasticmachine merge upstream

@madirey madirey mentioned this pull request Jul 13, 2020
Merged
1 task
@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020

hmm - sorry

@elasticmachine merge upstream

@madirey
Copy link
Contributor

madirey commented Jul 13, 2020

Related: #71449

@jbudz jbudz requested review from a team as code owners July 13, 2020 15:34
@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020

@madirey - thanks. I merged that commit in for a full snapshot verification.

@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020 •
edited
Loading

71449 is merged. Once this is green I'll revert the endpoint changes at 52d68dc, and the unverified snapshot changes at 4284ac3 for a verified green, and then merge.

@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020 

     │      Error: [illegal_argument_exception] composable template [generic-logs] template after composition is invalid
     │       at respond (node_modules/elasticsearch/src/lib/transport.js:349:15)

this one's unrelated, we'll address seperately. reverting unverfied snapshot and merging if green.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jbudz jbudz merged commit b3c6ce9 into elastic:master Jul 13, 2020
jbudz added a commit that referenced this pull request Jul 13, 2020
@jbudz @madirey
rm index: false from binary mappings (#71343)
3e9c3dc 
* rm index: false from binary mappings

* test against unverified snapshot

* two more

* Mapping adjustments

* Revert "Mapping adjustments"

This reverts commit 52d68dc.

* Revert "test against unverified snapshot"

This reverts commit 4284ac3.

Co-authored-by: Madison Caldwell <madison.caldwell@elastic.co>
@jbudz
Copy link
Member Author

jbudz commented Jul 13, 2020

7.9/x: 3e9c3dc

@andrewkroh andrewkroh mentioned this pull request Jul 13, 2020
Merged
@liza-mae liza-mae mentioned this pull request Jul 14, 2020
Closed
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 15, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 71343 or prevent reminders by adding the backport:skip label.

4 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 71343 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 71343 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 71343 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 71343 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jen-huang jen-huang jen-huang approved these changes

@tylersmalley tylersmalley tylersmalley approved these changes

Assignees
No one assigned
Labels
backported release_note:skip Skip the PR/issue when compiling release notes review Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jbudz @elasticmachine @tylersmalley @jen-huang @romseygeek @madirey @kibanamachine