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

Enable default fields for Elasticsearch 6.x #11205

Merged
merged 3 commits into from
Mar 12, 2019
Merged

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Mar 12, 2019

Previously default_fields were only enabled for Elasticsearch 7.x. For the migration it is required that also indices from Elasticsearch 6.x have the default_field option set. This enables it for 6.x except 6.0.

Closes #11187

Previously default_fields were only enabled for Elasticsearch 7.x. For the migration it is required that also indices from Elasticsearch 6.x have the default_field option set. This enables it for 6.x.

Closes elastic#11187
@ruflin
Copy link
Contributor Author

ruflin commented Mar 12, 2019

I tried to find a good reason on why we enabled this only for 7.0 and not 6.x. The initial PR was done here: #7015 It could be argued that it's a breaking change as non keyword / ip fields which were directly searchable before are not searchable anymore. I "assume" before you could have typed just 42 and it would have returned all fields with the value 42. This is now not possible anymore but from my perspective an ok tradeoff for this migration case.

@ruflin ruflin requested review from ph and urso March 12, 2019 07:23
@ruflin
Copy link
Contributor Author

ruflin commented Mar 12, 2019

I tested this against 6.6 and it works as expected. If I run this against 6.0 I get the following error:

2019-03-12T08:37:10.161+0100    ERROR   pipeline/output.go:100  Failed to connect to backoff(elasticsearch(http://localhost:9200)): Connection marked as failed because the onConnect callback failed: Error loading Elasticsearch template: could not load template. Elasticsearch returned: couldn't load template: couldn't load json. Error: 400 Bad Request: {"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"unknown setting [index.query.default_field.0] did you mean [index.query.default_field]?"}],"type":"illegal_argument_exception","reason":"unknown setting [index.query.default_field.0] did you mean [index.query.default_field]?"},"status":400}. Response body: {"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"unknown setting [index.query.default_field.0] did you mean [index.query.default_field]?"}],"type":"illegal_argument_exception","reason":"unknown setting [index.query.default_field.0] did you mean [index.query.default_field]?"},"status":400}.

I think the change was applied here: elastic/elasticsearch#26320 In 6.0 default_field did not accept an array yet. To keep 6.7 compatible with 6.0 I will exclude 6.0 from setting the default_field. For the migration, we should recommend users to be on 6.7 first anyways.

urso
urso previously approved these changes Mar 12, 2019
@urso urso dismissed their stale review March 12, 2019 11:38

Limit to 6.7 version only?

@@ -277,7 +277,7 @@ func buildIdxSettings(ver common.Version, userSettings common.MapStr) common.Map
indexSettings.Put("number_of_routing_shards", defaultNumberOfRoutingShards)
}

if ver.Major >= 7 {
if ver.Major >= 6 {
Copy link

Choose a reason for hiding this comment

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

This enables it for all 6.x ES versions. Would it be better if we check the minor version as well?

ver.Major >= 7 || (ver.Major ==6 && ver.Minor >= 7)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have pushed by local changes. My take at the moment is that it should set it for 6.x versions except 6.0.

@ph
Copy link
Contributor

ph commented Mar 12, 2019

@urso Targetting 6.x look a good idea to me, following @joshdover this option exist since 5.6

@ruflin ruflin merged commit 06b6ff2 into elastic:6.7 Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants