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

Default `include_in_all` for numeric-like types to false #20085

Merged
merged 2 commits into from Aug 22, 2016

Conversation

Projects
None yet
6 participants
@dakrone
Member

dakrone commented Aug 19, 2016

This includes:

  • All regular numeric types such as int, long, scaled-float, double, etc
  • IP addresses
  • Dates
  • Geopoints and Geoshapes

Relates to #19784

Default `include_in_all` for numeric-like types to false
This includes:

- All regular numeric types such as int, long, scaled-float, double, etc
- IP addresses
- Dates
- Geopoints and Geoshapes

Relates to #19784
@@ -186,7 +186,8 @@ public void testIgnoreMalformed() throws Exception {
public void testIncludeInAll() throws Exception {
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties").startObject("field").field("type", "ip").endObject().endObject()
.startObject("properties").startObject("field").field("type", "ip")
.field("include_in_all", true).endObject().endObject()

This comment has been minimized.

@rjernst

rjernst Aug 19, 2016

Member

These tests now check that overriding to true works. Can we have tests for the default, ie that by default the field is not added to _all?

@rjernst

rjernst Aug 19, 2016

Member

These tests now check that overriding to true works. Can we have tests for the default, ie that by default the field is not added to _all?

This comment has been minimized.

@dakrone

dakrone Aug 19, 2016

Member

That's the blurb directly below this, previously it checked that it was included in _all by default first (without setting the setting), and then that it wasn't when the setting was false; now it checks that it's included in _all when explicitly set and is not included when unset entirely.

@dakrone

dakrone Aug 19, 2016

Member

That's the blurb directly below this, previously it checked that it was included in _all by default first (without setting the setting), and then that it wasn't when the setting was false; now it checks that it's included in _all when explicitly set and is not included when unset entirely.

@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Aug 19, 2016

Member

LGTM. Two minor suggestions.

Member

rjernst commented Aug 19, 2016

LGTM. Two minor suggestions.

@@ -204,8 +205,7 @@ public void testIncludeInAll() throws Exception {
assertEquals("::1", fields[0].stringValue());
mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
.startObject("properties").startObject("field").field("type", "ip")
.field("include_in_all", false).endObject().endObject()
.startObject("properties").startObject("field").field("type", "ip").endObject().endObject()

This comment has been minimized.

@dakrone

dakrone Aug 19, 2016

Member

This is for checking that it is not included when unset

@dakrone

dakrone Aug 19, 2016

Member

This is for checking that it is not included when unset

This comment has been minimized.

@rjernst

rjernst Aug 19, 2016

Member

Oops, I missed this. Thanks!

@rjernst

rjernst Aug 19, 2016

Member

Oops, I missed this. Thanks!

@dakrone dakrone merged commit b6ec1ae into elastic:master Aug 22, 2016

2 checks passed

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

@dakrone dakrone removed the discuss label Aug 22, 2016

@epixa

This comment has been minimized.

Show comment
Hide comment
@epixa

epixa Aug 22, 2016

Member

Sorry for waiting until this was merged - I mistakenly thought this was blocked on the discussion in the linked ticket.

Is there any chance we can hold off doing this change until 6.0? If this stays in, the Kibana team is going to need to block the 5.0 release for a currently-unknown amount of time while we figure out how best to handle this on our end.

Member

epixa commented Aug 22, 2016

Sorry for waiting until this was merged - I mistakenly thought this was blocked on the discussion in the linked ticket.

Is there any chance we can hold off doing this change until 6.0? If this stays in, the Kibana team is going to need to block the 5.0 release for a currently-unknown amount of time while we figure out how best to handle this on our end.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Aug 23, 2016

Member

This was reverted in 3298a4e, and will be re-merged once the 6.0 branch has been created.

Member

dakrone commented Aug 23, 2016

This was reverted in 3298a4e, and will be re-merged once the 6.0 branch has been created.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Aug 23, 2016

Contributor

@dakrone thanks for working on it so quickly. We had a discussion about this in our status meeting with half of the team and decided to back this out until we branched off 5.x and master becomes 6.0. This gives us enough time to find a better solution for kibana. It's too close to feature freeze to fix this on the kibana end. Also, what came up in the discussion is that we want to remove _all completely and several interesting ideas came up how to solve it. I suggested for instance to have some metadata on the index creation that leads to different defaults (ie. a template triggered by some metadata) or maybe a plugin people need to install.

Contributor

s1monw commented Aug 23, 2016

@dakrone thanks for working on it so quickly. We had a discussion about this in our status meeting with half of the team and decided to back this out until we branched off 5.x and master becomes 6.0. This gives us enough time to find a better solution for kibana. It's too close to feature freeze to fix this on the kibana end. Also, what came up in the discussion is that we want to remove _all completely and several interesting ideas came up how to solve it. I suggested for instance to have some metadata on the index creation that leads to different defaults (ie. a template triggered by some metadata) or maybe a plugin people need to install.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Sep 8, 2016

Member

I have re-pushed this to the master branch now that master is 6.0

Member

dakrone commented Sep 8, 2016

I have re-pushed this to the master branch now that master is 6.0

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Sep 28, 2016

Member

I have re-reverted this on the master branch :)

Member

dakrone commented Sep 28, 2016

I have re-reverted this on the master branch :)

@epixa

This comment has been minimized.

Show comment
Hide comment
@epixa

epixa Sep 29, 2016

Member

@dakrone What's the plan with this now?

Member

epixa commented Sep 29, 2016

@dakrone What's the plan with this now?

@javanna javanna removed the v6.0.0-alpha1 label Sep 30, 2016

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Oct 5, 2016

Member

@epixa see the design/discussion I posted on #19784 (comment)

Member

dakrone commented Oct 5, 2016

@epixa see the design/discussion I posted on #19784 (comment)

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