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

Remove Adjacency_matrix setting #46327

Merged
merged 1 commit into from Sep 19, 2019

Conversation

@markharwood
Copy link
Contributor

commented Sep 4, 2019

Now that PR #46257 has improved internal memory usage we no longer need the default limit of 100 filters defined by index.max_adjacency_matrix_filters. This PR removes the setting and uses the existing indices.query.bool.max_clause_count setting for Boolean clause limits to apply to the number of filters used in an adjacency matrix.

Closes #46324

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Sep 4, 2019

@markharwood markharwood requested a review from jpountz Sep 5, 2019

@markharwood

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@jpountz this is the removal of the setting in 8.0 we talked about. Are there additional BWC things I need to add relating to dealing with mixed version clusters or opening old indices?

@@ -198,13 +198,13 @@ public String separator() {
@Override
protected AggregatorFactory doBuild(SearchContext context, AggregatorFactory parent, Builder subFactoriesBuilder)
throws IOException {
int maxFilters = context.indexShard().indexSettings().getMaxAdjacencyMatrixFilters();
int maxFilters = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.get(context.indexShard().indexSettings().getSettings());

This comment has been minimized.

Copy link
@jpountz

jpountz Sep 5, 2019

Contributor

I don't think this is correct since you are looking up a node setting in the index settings?

This comment has been minimized.

Copy link
@markharwood

markharwood Sep 5, 2019

Author Contributor

UPDATE: @jpountz I got this from QueryParserHelper and it looks like that would have this issue.

However, testing it with an abusive query string/index shows that it does pick up the correct setting from the elasticsearch.yml file so not sure how that magic works its way through.

This comment has been minimized.

Copy link
@markharwood

markharwood Sep 5, 2019

Author Contributor

Presumably we just substitute for BooleanQuery.getMaxClauseCount() ?

This comment has been minimized.

Copy link
@markharwood

markharwood Sep 9, 2019

Author Contributor

@jpountz I updated the PR to use BooleanQuery.getMaxClauseCount() as the source of truth. Look good to you?
@romseygeek thought that would be the right thing to do.

This comment has been minimized.

Copy link
@jpountz

jpountz Sep 12, 2019

Contributor

👍

@markharwood markharwood force-pushed the markharwood:fix/46324 branch from b6c32bf to 911fc0b Sep 12, 2019

@markharwood

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

(Answering my own question here:) indices created in 7.x with the index.max_adjacency_matrix_filters setting set can be opened in 8.x - the setting is just moved into an "archived" part of the settings rather than throwing any error.

@markharwood markharwood force-pushed the markharwood:fix/46324 branch 6 times, most recently from 784a664 to f718cb5 Sep 12, 2019

@markharwood markharwood force-pushed the markharwood:fix/46324 branch from f718cb5 to e782116 Sep 19, 2019

@markharwood markharwood added the v8.0.0 label Sep 19, 2019

@markharwood markharwood merged commit dc0abec into elastic:master Sep 19, 2019

9 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docs Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.