-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix mappings for Upgrade Assistant reindexOperationSavedObjectType. #71710
Fix mappings for Upgrade Assistant reindexOperationSavedObjectType. #71710
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@@ -15,13 +15,25 @@ export const reindexOperationSavedObjectType: SavedObjectsType = { | |||
mappings: { | |||
properties: { | |||
reindexTaskId: { | |||
type: 'keyword', | |||
type: 'text', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I made these other changes to regain parity with how the dynamic mapping was originally mapping these fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the default mappings for a dynamic field detected as being text, but is there any value in maintaining parity? With this mapping two fields will be added, text and keyword. It seems unlikely that we would do a match query on the keyword, so maybe having only a text field (with index: false
) would add less indexing overhead without removing any validation by ES.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising this and for suggesting a solution! I decided to maintain parity to minimize the risk of introducing another regression as much as possible. I have no idea if there's any value beyond that for maintaining parity (haven't looked into it yet). I agree that many of these fields probably don't need to be indexed as keyword (or possibly at all). #64547 tracks the work of updating these mappings to minimize the number of fields we're indexing and I'll keep your suggestion in mind when I begin working on it.
type: 'keyword', | ||
ignore_above: 256, | ||
}, | ||
}, | ||
}, | ||
reindexTaskPercComplete: { | ||
type: 'float', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally being dynamically mapped as long
but I believe that was an error, since we expect floating point values where this field is consumed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💔 Build Failed
Failed CI Steps
Build metricsHistory
To update your PR or re-run it, just comment with: |
Fixes #71642.
Release note
Some users who upgrade to 7.8.0 won't be able to start Kibana if previous upgrades resulted in errors with very large error messages. In this situation, they'll see a Kibana log error like this:
This fixes the problem so that users won't see this error and will be able to start Kibana as usual.
Steps to test
To test, you need to first reproduce the error in 7.8.0 and then verify that this change fixes it. Download ES 7.7.0, Kibana 7.7.0, ES 7.8.0, and Kibana 7.8.0.
elasticsearch-7.7.0/data
over toelasticsearch-7.8.0/data
.yarn es snapshot
to install ES and kill the process. Copyelasticsearch-7.7.0/data
tokibana/.es/7.8.1/data
.xpack.security.enabled: false
to their respective config files.kibana/.es/7.8.1/bin/elasticsearch
and start Kibana withyarn start
. Kibana will start normally.GET .kibana_1/_doc/upgrade-assistant-reindex-operation:fb36e570-bca7-11e9-bbfd-f767236c4a9b
.Fatal error
To-do
In a subsequent PR, I'll add a test that verifies that we can ingest the mock document without any errors. Addressed in #72347.