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

Removing explicit SearchResponse usages in tests - v3 #102019

Conversation

pmpailis
Copy link
Contributor

@pmpailis pmpailis commented Nov 10, 2023

Tests covered in this PR:

  • org.elasticsearch.percolator.PercolatorQuerySearchIT

@pmpailis pmpailis added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Nov 10, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@pmpailis pmpailis removed the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 10, 2023
@JVerwolf
Copy link
Contributor

@elasticmachine update branch

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

Looks good thanks! Lets just back out the multi-search response bit.

assertThat(item.getFailureMessage(), notNullValue());
assertThat(item.getFailureMessage(), containsString("[test/6] couldn't be found"));
} finally {
if (response != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Lets back this one out please, this should be resolved by adjusting the ref counting on MultiSearchResponse in production so this becomes a single .decRef(), not by us doing this manually in tests :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, makes sense. Was wondering whether we want to eliminate as many test instances as possible in this round and then revisit explicitly MultiSearchResponse when we have the decRef properly implement. Reverted.

@pmpailis pmpailis added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 13, 2023
@pmpailis
Copy link
Contributor Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit 8f108ec into elastic:main Nov 13, 2023
13 checks passed
@pmpailis pmpailis deleted the fix/remove-explicit-search-response-test-v3 branch November 13, 2023 12:33
davidkyle pushed a commit to davidkyle/elasticsearch that referenced this pull request Nov 13, 2023
Tests covered in this PR:

* `org.elasticsearch.percolator.PercolatorQuerySearchIT`
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :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.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants