Skip to content

Conversation

@javanna
Copy link
Member

@javanna javanna commented Oct 18, 2022

Stop testing that the limit gets hit when the value is the default one, as that causes a lot of objects being created which can cause OOM when executing the test. Instead, added a specific test to verify that the default value is being set appropriately based on its corresponding setting.

Closes #90984
Closes #90985
Closes #90986

Stop testing that the limit gets hit when the value is the default one,
as that causes a lot of objects being created which can cause OOM when
executing the test. Instead, added a specific test to verify that the
default value is being set appropriately based on its corresponding setting.

Closes elastic#90984
Closes elastic#90985
Closes elastic#90986
@javanna javanna added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories v8.6.0 labels Oct 18, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Oct 18, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)


private static int maxNestedDepth = 20;
//We set the default value for tests that don't go through SearchModule
private static int maxNestedDepth = SearchModule.INDICES_MAX_CLAUSE_COUNT_SETTING.getDefault(Settings.EMPTY);
Copy link
Member

Choose a reason for hiding this comment

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

INDICES_MAX_CLAUSE_COUNT_SETTING is 4096, which I don't think is correct.

I think you want INDICES_MAX_NESTED_DEPTH_SETTING here, which has a default value of 30 (changed in #90425)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes indeed, thanks for catching that. 4096 seems a bit high :) luckily this is only for tests. I tried to refactor it because I don't really like the static field and the current logic, but it's not so straight-forward to do.

Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Just so it's OK that the previous change specifically set maxNestedDepth to 20 when the actual default in the setting was 30. I do not know if that was intentional.

@javanna
Copy link
Member Author

javanna commented Oct 18, 2022

Just so it's OK that the previous change specifically set maxNestedDepth to 20 when the actual default in the setting was 30. I do not know if that was intentional.

That default value was being overwritten within SearchModule and got out of sync with the actual setting default which was recently raised to 30. This was an attempt to align the two.

@javanna javanna merged commit 447afae into elastic:main Oct 18, 2022
@javanna
Copy link
Member Author

javanna commented Oct 18, 2022

Thanks for the review @benwtrent !

javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 7, 2022
This commit holds the same change as elastic#90990 but for WrapperQueryBuilderTests,
which has special needs and overrides the default impl of the test method

Closes elastic#91342
javanna added a commit that referenced this pull request Nov 7, 2022
This commit holds the same change as #90990 but for WrapperQueryBuilderTests,
which has special needs and overrides the default impl of the test method

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

Labels

:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v8.6.0

Projects

None yet

3 participants