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.

Sign up for GitHub

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

Jump to bottom

Fixing exists query REST tests for sparse_vector_fields #100030

Merged
merged 12 commits into from
Oct 6, 2023
Merged

Fixing exists query REST tests for sparse_vector_fields #100030

merged 12 commits into from
Oct 6, 2023

Conversation

pmpailis
Copy link
Contributor

@pmpailis pmpailis commented Sep 28, 2023 •
edited
Loading

Removing (incorrectly) expected exceptions for exists queries sparse_vectors in < 8.0.0 versions.

Closes #100003

@pmpailis pmpailis added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories labels Sep 28, 2023
@elasticsearchmachine elasticsearchmachine added Team:Search Meta label for search team v8.11.0 labels Sep 28, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@pmpailis pmpailis requested a review from benwtrent September 29, 2023 08:56
benwtrent
benwtrent reviewed Sep 30, 2023
View reviewed changes
rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.vectors/90_sparse_vector.yml Outdated
Comment on lines 166 to 174
- do:
catch: /\[sparse_vector\] fields do not support \[exists\] queries/
search:
rest_total_hits_as_int: true
index: test
body:
query:
exists:
field: ml.tokens
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't remove this. Behavior for 8.0.0-8.11.0 should not be changed.

Additionally, the test says ""Sparse vector in 7.x": and now you are restricting it to never be 7.x. That seems weird.

Please adjust the test so that it only tests 7.x & add one for [8.0.0-8.11.0) that includes the exists clause failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was more of a retraction commit tbh, removing a test from the 7.x section introduced in the original PR for exists queries in sparse vectors for 8.11.

I was trying to limit the effect of the mixed node cluster setup just in the tests and try to account for the different scenarios under separate requests, experimenting also with the node_selector - but couldn't find any similar cases w/o any modifications to the codebase as well. I also tried limiting the nodes & setting num_replicas=0 when creating the index for the 7.x scenario, however this would pretty much defeat the purpose of any cluster/bcw test altogether. The issue with the node selector, AFAICT, was that when running the test we might end up with a replica placed on a recently upgraded 8.11 index, which later caused all searches that landed on that node to fail, as the index version was less than the expected one for the mixed cluster, raising the exception, even when we had explicitly defined the node_selector for the search request.

Ended up slightly updating the codebase as well, to return a deprecation warning along with the headers, and treat incoming 7.x exists queries for sparse_vector fields as unmatchable ones on >= 8.11 (i.e. return a MatchNoDocsQuery instead). Happy to remove the deprecation & fallback to raising IllegalArgumentException in all cases, but would need some help in adjusting it with the existing restapi-test params, as I'm not familiar with the setup & alternatively, I had to do some refactoring on the tests to make it pass consistently. 🤔

I also added an explicit case for [8.0, 8.11) versions though - expecting such requests to fail in all cases - which should be fine as the behaviour will be consistent for the mixed-cluster test suite.

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 3, 2023

@elasticsearchmachine test this please

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 3, 2023

@elasticmachine update branch

@benwtrent benwtrent added the test-full-bwc Trigger full BWC version matrix tests label Oct 3, 2023
@benwtrent
Copy link
Member

@elasticmachine update branch

benwtrent
benwtrent approved these changes Oct 4, 2023
View reviewed changes
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.

Let's run the fully bwc-suite to make sure. But this looks good.

@benwtrent benwtrent added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport-and-merge v8.12.0 labels Oct 4, 2023
@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

@elasticmachine update branch

@mattc58 mattc58 removed the v8.11.0 label Oct 4, 2023
@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

@elasticmachine run elasticsearch-ci/part-2

@benwtrent
Copy link
Member

At least one of the BWC failures is a "version bump" failure:

./gradlew ':x-pack:qa:full-cluster-restart:v8.11.0#bwcTest' -Dtests.class="org.elasticsearch.xpack.restart.WatcherMappingUpdateIT" -Dtests.method="testMappingsAreUpdated {cluster=UPGRADED}" -Dtests.seed=9450A86123288E8A -Dtests.bwc=true -Dtests.locale=hr -Dtests.timezone=America/Dawson_Creek -Druntime.java=21

Expected: a string containing "\"version\":\"8.12.0\""
09:26:45          but: was "{".watches":{"mappings":{"dynamic":"strict","_meta":{"managed_index_mappings_version":1,"version":"8.11.0"},"properties":{"actions":{"type":"object","dynamic":"true","enabled":false},"condition":{"type":"object","dynamic":"true","enabled":false},"input":{"type":"object","dynamic":"true","enabled":false},"metadata":{"type":"object","dynamic":"true"},"status":{"type":"object","dynamic":"true","enabled":false},"throttle_period":{"type":"keyword","index":false,"doc_values":false},"throttle_period_in_millis":{"type":"long","index":false,"doc_values":false},"transform":{"type":"object","dynamic":"true","enabled":false},"trigger":{"type":"object","dynamic":"true","enabled":false}}}}}"

The others seem like timeouts unrelated to this change:

./gradlew ':x-pack:qa:rolling-upgrade:v8.2.3#twoThirdsUpgradedTest' -Dtests.class="org.elasticsearch.upgrades.IndexingIT" -Dtests.method="testIndexing" -Dtests.seed=62101E3FB48C7DAF -Dtests.bwc=true -Dtests.locale=fr-FR -Dtests.timezone=America/Belem -Druntime.java=21

So we will need to update this branch as those things are fixed so we can get a green build.

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

Thanks @benwtrent for taking a look at it! Will update and hopefully avoid the timeouts in the next run :)

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

@elasticmachine run elasticsearch-ci/full-bwc

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 4, 2023

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 5, 2023

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 5, 2023 •
edited
Loading

More randomly occurring errors related to twoThirdsUpgradedTest. Will try once more and in the meantime will take a look locally and see if there're any issues I can spot.

https://gradle-enterprise.elastic.co/s/rzxcqgaxgidsu

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 5, 2023

@elasticmachine run elasticsearch-ci/full-bwc

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 6, 2023

@elasticmachine update branch

@pmpailis
Copy link
Contributor Author

pmpailis commented Oct 6, 2023

@elasticmachine run elasticsearch-ci/full-bwc

@benwtrent
Copy link
Member

Looking at the failures in full-bwc, none are due to this change.

Importantly, bwc tests pass with 7.latest, 8.something_not_10_or_11, 8.10, 8.11.

Lets merge it.

@benwtrent benwtrent merged commit 24353a9 into elastic:main Oct 6, 2023
@pmpailis pmpailis deleted the fix/100003 branch October 6, 2023 11:55
@pmpailis pmpailis mentioned this pull request Oct 6, 2023
Merged
pmpailis added a commit to pmpailis/elasticsearch that referenced this pull request Oct 6, 2023
@pmpailis
Fixing exists query REST tests for sparse_vector_fields (elastic#100030)
03cd1fd 
Removing (incorrectly) expected exceptions for `exists` queries `sparse_vectors`  in  < 8.0.0 versions.

Closes elastic#100003
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 8.11

elasticsearchmachine pushed a commit that referenced this pull request Oct 6, 2023
@pmpailis
Fixing exists query REST tests for sparse_vector_fields (#100030) (#1…
a3d9e72 
…00397)

Removing (incorrectly) expected exceptions for `exists` queries `sparse_vectors`  in  < 8.0.0 versions.

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

@benwtrent benwtrent benwtrent approved these changes

Assignees
No one assigned
Labels
auto-merge-without-approval 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 test-full-bwc Trigger full BWC version matrix tests v8.11.1 v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CI] MixedClusterClientYamlTestSuiteIT test {p0=search.vectors/90_sparse_vector/Sparse vector in 7.x} failing
5 participants
@pmpailis @elasticsearchmachine @benwtrent @mattc58 @elasticmachine