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

Disable _all by default, disallow configuring _all on 6.0+ indices #22144

Merged
merged 1 commit into from Jan 12, 2017

Conversation

Projects
None yet
5 participants
@dakrone
Member

dakrone commented Dec 13, 2016

This change disables the _all meta field by default.

Now that we have the "all-fields" method of query execution, we can save both
indexing time and disk space by disabling it.

_all can no longer be configured for indices created after 6.0.

Relates to #20925 and #21341
Resolves #19784

@jimczi

Just left some minor things that need clarifications.
Other than that this change looks good. I am surprised that it did not require more changes in the tests but I guess that the all_fields mode covers most of the use case.

Show outdated Hide outdated core/src/test/java/org/elasticsearch/index/mapper/AllFieldMapperTests.java
Show outdated Hide outdated ...g/elasticsearch/search/fetch/subphase/highlight/HighlighterSearchIT.java
Show outdated Hide outdated core/src/test/java/org/elasticsearch/search/nested/SimpleNestedIT.java
Show outdated Hide outdated core/src/test/java/org/elasticsearch/search/query/SimpleQueryStringIT.java
Show outdated Hide outdated docs/reference/migration/migrate_6_0/mappings.asciidoc
Show outdated Hide outdated docs/reference/migration/migrate_6_0/mappings.asciidoc
"query": {
"query_string": {
"query": "post_date:foo",
"lenient": false

This comment has been minimized.

@jimczi

jimczi Dec 13, 2016

Member

false is still the default value, so why is this needed ?

@jimczi

jimczi Dec 13, 2016

Member

false is still the default value, so why is this needed ?

This comment has been minimized.

@dakrone

dakrone Dec 13, 2016

Member

query_string defaults to all-fields mode if _all is disabled, which turns on lenient automatically, so this is needed in order to make the query non-valid.

@dakrone

dakrone Dec 13, 2016

Member

query_string defaults to all-fields mode if _all is disabled, which turns on lenient automatically, so this is needed in order to make the query non-valid.

"query_string": {
"query": "post_date:foo",
"lenient": false
}

This comment has been minimized.

@jimczi

jimczi Dec 13, 2016

Member

same here

@jimczi

jimczi Dec 13, 2016

Member

same here

This comment has been minimized.

@dakrone

dakrone Dec 13, 2016

Member

Same as above :)

@dakrone

dakrone Dec 13, 2016

Member

Same as above :)

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 13, 2016

Member

I pushed some commits, thanks for looking @jimczi!

Member

dakrone commented Dec 13, 2016

I pushed some commits, thanks for looking @jimczi!

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Dec 14, 2016

Contributor

I don't like having a feature that has such a large impact on the code base disabled by default, as it means the maintenance cost remains the same (=high), while the benefit to our users decreases significantly, as most of them will not opt in. Two possible paths that I can think of are either to remove it entirely (ie. forbid 6.x indices from enabling it and removing in 7), or keep _all as a simple text field that is used as a name convention for a catch-all field that other fields can copy to.

Contributor

jpountz commented Dec 14, 2016

I don't like having a feature that has such a large impact on the code base disabled by default, as it means the maintenance cost remains the same (=high), while the benefit to our users decreases significantly, as most of them will not opt in. Two possible paths that I can think of are either to remove it entirely (ie. forbid 6.x indices from enabling it and removing in 7), or keep _all as a simple text field that is used as a name convention for a catch-all field that other fields can copy to.

@jimczi

This comment has been minimized.

Show comment
Hide comment
@jimczi

jimczi Dec 14, 2016

Member

I don't like having a feature that has such a large impact on the code base disabled by default

The impact is pretty contained and should not change the OOB significantly. Though I agree that keeping the feature is costly so I think that forbids in 6 and removing in 7 is a good plan.

or keep _all as a simple text field that is used as a name convention for a catch-all field that other fields can copy to.

Not sure it would be useful if we don't copy to it by default and if we always use the all_fields mode in the query_string. It's really an OOB issue which seems to be resolved by the all_fields mode.

Member

jimczi commented Dec 14, 2016

I don't like having a feature that has such a large impact on the code base disabled by default

The impact is pretty contained and should not change the OOB significantly. Though I agree that keeping the feature is costly so I think that forbids in 6 and removing in 7 is a good plan.

or keep _all as a simple text field that is used as a name convention for a catch-all field that other fields can copy to.

Not sure it would be useful if we don't copy to it by default and if we always use the all_fields mode in the query_string. It's really an OOB issue which seems to be resolved by the all_fields mode.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 14, 2016

Member

Alright, sounds like the consensus is to disallow in 6.0+ and we can eventually remove it in 7.0, I'll update the PR and title.

Member

dakrone commented Dec 14, 2016

Alright, sounds like the consensus is to disallow in 6.0+ and we can eventually remove it in 7.0, I'll update the PR and title.

@dakrone dakrone changed the title from Disable _all by default to Disable _all by default, disallow configuring _all on 6.0+ indices Jan 8, 2017

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 8, 2017

Member

@jpountz @jimczi okay, I've changed this to disallow configuring _all for indices created 6.0+ in addition to it being disabled by default. Sorry for the large PR, but it was mostly removing tests that are no longer needed.

Member

dakrone commented Jan 8, 2017

@jpountz @jimczi okay, I've changed this to disallow configuring _all for indices created 6.0+ in addition to it being disabled by default. Sorry for the large PR, but it was mostly removing tests that are no longer needed.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 10, 2017

Member

retest this please

Member

dakrone commented Jan 10, 2017

retest this please

Show outdated Hide outdated docs/reference/migration/migrate_6_0/mappings.asciidoc
@jimczi

Thanks @dakrone
This looks good. I can imagine how painful it was to understand why all this tests were relying on _all and how to remove it without loosing the test case.
Though I think we should at least keep some test around regarding the AllFieldType and maybe the copyToAll functionality. It will surely be removed but in the mean time we may add things that breaks it.

IMO we should completely disallow enabling _all even if the index was created < 6.x. We don't want to create more _all users ;)

Show outdated Hide outdated ...va/org/elasticsearch/action/admin/indices/template/BWCTemplateTests.java
Show outdated Hide outdated docs/reference/migration/migrate_6_0/mappings.asciidoc
@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 11, 2017

Member

@jimczi I re-added some tests based on your feedback, thanks for taking a look!

regarding the AllFieldType and maybe the copyToAll functionality

Can you describe what you mean here? Do you mean just that fields are copied to the _all field automatically?

Member

dakrone commented Jan 11, 2017

@jimczi I re-added some tests based on your feedback, thanks for taking a look!

regarding the AllFieldType and maybe the copyToAll functionality

Can you describe what you mean here? Do you mean just that fields are copied to the _all field automatically?

@jimczi

This comment has been minimized.

Show comment
Hide comment
@jimczi

jimczi Jan 11, 2017

Member

Can you describe what you mean here? Do you mean just that fields are copied to the _all field automatically?

Yes and the AllFieldTypeTest. Why is it removed ?

Member

jimczi commented Jan 11, 2017

Can you describe what you mean here? Do you mean just that fields are copied to the _all field automatically?

Yes and the AllFieldTypeTest. Why is it removed ?

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 11, 2017

Member

the AllFieldTypeTest. Why is it removed ?

My accidental over-zealousness, I've re-added it :)

As far as testing that fields are copied, the _all field is still used for the query_string and simple_query_string tests, which tests that the fields are copied to the _all field (since it queries on them)

Member

dakrone commented Jan 11, 2017

the AllFieldTypeTest. Why is it removed ?

My accidental over-zealousness, I've re-added it :)

As far as testing that fields are copied, the _all field is still used for the query_string and simple_query_string tests, which tests that the fields are copied to the _all field (since it queries on them)

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Jan 11, 2017

Member

retest this please

Member

dakrone commented Jan 11, 2017

retest this please

@jimczi

jimczi approved these changes Jan 11, 2017

LGTM

Disable _all by default
This change disables the _all meta field by default.

Now that we have the "all-fields" method of query execution, we can save both
indexing time and disk space by disabling it.

_all can no longer be configured for indices created after 6.0.

Relates to #20925 and #21341
Resolves #19784

@dakrone dakrone merged commit 7a18bb5 into elastic:master Jan 12, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details

@dakrone dakrone deleted the dakrone:disable-all-by-default branch Jan 23, 2017

exekias added a commit to exekias/beats that referenced this pull request Mar 21, 2017

Remove `_all` field from template for ES >= 6.0
ES 6.0 deprecates `_all`, see elastic/elasticsearch#22144.

Previous `*.template.json` have been moved to `*.template-es5x.json` and
a new version for 6.X is added as the latest version.

exekias added a commit to exekias/beats that referenced this pull request Mar 22, 2017

Remove `_all` field from template for ES >= 6.0
ES 6.0 deprecates `_all`, see elastic/elasticsearch#22144.

`*.template-es6x.json` has been added in case Elasticsearch 6.X is
detected

tsg added a commit to elastic/beats that referenced this pull request Mar 22, 2017

Remove `_all` field from template for ES >= 6.0 (#3778)
ES 6.0 deprecates `_all`, see elastic/elasticsearch#22144.

`*.template-es6x.json` has been added in case Elasticsearch 6.X is
detected
@lukas-vlcek

This comment has been minimized.

Show comment
Hide comment
@lukas-vlcek

lukas-vlcek Apr 3, 2017

Contributor

If I want to index documents that have _all field in the top level would it be possible to configure _all field in mapping? AFAIK it is currently not possible to index documents having _all field but in the future, when the historical function of _all field is removed, wouldn't it make sense to threat this field as any other document field?

In other words, will it be possible to index documents like the following one OOB?

{
  "foo": "bar",
  "_all": "dummy"
}
Contributor

lukas-vlcek commented Apr 3, 2017

If I want to index documents that have _all field in the top level would it be possible to configure _all field in mapping? AFAIK it is currently not possible to index documents having _all field but in the future, when the historical function of _all field is removed, wouldn't it make sense to threat this field as any other document field?

In other words, will it be possible to index documents like the following one OOB?

{
  "foo": "bar",
  "_all": "dummy"
}
@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Apr 3, 2017

Member

In other words, will it be possible to index documents like the following one OOB?

We strongly recommend not naming user-created fields started with an _, as those are reserved for internal use.

You could, however, name it my_all (or something else) and use the copy_to feature on all other mapped fields to create your own version of an _all field.

Member

dakrone commented Apr 3, 2017

In other words, will it be possible to index documents like the following one OOB?

We strongly recommend not naming user-created fields started with an _, as those are reserved for internal use.

You could, however, name it my_all (or something else) and use the copy_to feature on all other mapped fields to create your own version of an _all field.

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Aug 15, 2017

Add deprecation logging when _all is enabled
In 6.0 `_all` is not configurable and disabled by default.

Relates to #22144

dakrone added a commit that referenced this pull request Aug 16, 2017

Add deprecation logging when _all is enabled (#26228)
* Add deprecation logging when _all is enabled

In 6.0 `_all` is not configurable and disabled by default.

Relates to #22144

* Log deprecation regardless of whether _all is enabled or disabled

* Add expected warnings in REST tests

* Assert warnings in unit tests

* Add warning check to various documentation tests

* Add assertWarnings if randomBoolean() is true for enabled

dakrone added a commit that referenced this pull request Aug 16, 2017

Add deprecation logging when _all is enabled (#26228)
* Add deprecation logging when _all is enabled

In 6.0 `_all` is not configurable and disabled by default.

Relates to #22144

* Log deprecation regardless of whether _all is enabled or disabled

* Add expected warnings in REST tests

* Assert warnings in unit tests

* Add warning check to various documentation tests

* Add assertWarnings if randomBoolean() is true for enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment