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

Register `indices.query.bool.max_clause_count` setting #18341

Merged
merged 4 commits into from May 19, 2016

Conversation

Projects
None yet
5 participants
@s1monw
Copy link
Contributor

commented May 13, 2016

This commit registers indices.query.bool.max_clause_count as a node
level setting and removes support for its synonym setting
index.query.bool.max_clause_count.

Closes #18336

Register `indices.query.bool.max_clause_count` setting
This commit registers `indices.query.bool.max_clause_count` as a node
level setting and removes support for its synonym setting
`index.query.bool.max_clause_count`.

Closes #18336
@rjernst

This comment has been minimized.

Copy link
Member

commented May 13, 2016

Lgtm

@dakrone dakrone added the >breaking label May 13, 2016

@bleskes

This comment has been minimized.

Copy link
Member

commented May 14, 2016

Can we maybe add a little explaination to the breaking change as to why it has to be a node level setting, despite of the fact that at face value it should be an index level and isn't really node dependent? (i.e., Lucene has this as a static thing, so it has to be like this). Also, I see it is marked as deprecated, is there is any other better way to accomplish this/plans to make one? if so it will be nice to mention that as well.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2016

@bleskes I don't get what you mean it was never a index level setting. it just had the confusing prefix.

@rjernst

This comment has been minimized.

Copy link
Member

commented May 15, 2016

I wonder if we should (in addition to this good rename) move the settings validation check in SettingsModule before the check for index level settings existing in node settings? This improves the output here so we get "did you mean" behavior with the old setting like this:

[elasticsearch] Exception in thread "main" java.lang.IllegalArgumentException: unknown setting [index.query.bool.max_clause_count] did you mean [indices.query.bool.max_clause_count]?

Otherwise with this change we get a confusing message stating the setting should be moved to indexes:

[elasticsearch] Please ensure all required values are updated on all indices by executing:
[elasticsearch] curl -XPUT 'http://localhost:9200/_all/_settings?preserve_existing=true' -d '{
[elasticsearch] "index.query.bool.max_clause_count" : "200"
[elasticsearch] }'
@bleskes

This comment has been minimized.

Copy link
Member

commented May 15, 2016

it was never a index level setting. it just had the confusing prefix.

Oh , you are of course right. Got confused by the confusing prefix. Sorry for the noise re that.

My question re-deprecation still stands though - is there (a plan for) an alternative configuration option?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2016

My question re-deprecation still stands though - is there (a plan for) an alternative configuration option?

I think there aren't any satisfying ways to fix this. For instance you would need to add a setter to anything that can rewrite to BQ which can set a setter on BQ to pass on the setting. One way of doing it would be a thread local which sucks too but I think that's the reason for the simple but global way of setting it.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2016

I wonder if we should (in addition to this good rename) move the settings validation check in SettingsModule before the check for index level settings existing in node settings? This improves the output here so we get "did you mean" behavior with the old setting like this:

@rjernst I think the check it correct there otherwise you won't get past that point since the other leftover index.* settings you have in the node settings will cause failures otherwise. I think I will just special case that setting there?

@rjernst

This comment has been minimized.

Copy link
Member

commented May 15, 2016

I think I will just special case that setting there?

Sure, sounds good.

@bleskes

This comment has been minimized.

Copy link
Member

commented May 17, 2016

I think there aren't any satisfying ways to fix this.

Sure. Does this mean we should not mark the settings as deprecated?

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2016

Sure. Does this mean we should not mark the settings as deprecated?

I don't see where this PR deprecates anything? I am confused how is this related?

@bleskes

This comment has been minimized.

Copy link
Member

commented May 18, 2016

I don't see where this PR deprecates anything?

The reason why I mentioned is the usage of Setting.Property.Deprecated here.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2016

The reason why I mentioned is the usage of Setting.Property.Deprecated here.

how the fuck.... not sure how that got there - not intended. That explains the confusion :) I will fix and push

s1monw added some commits May 19, 2016

@s1monw s1monw merged commit d77c299 into elastic:master May 19, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:issues/18336 branch May 19, 2016

@clintongormley clintongormley removed the >bug label May 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.