Skip to content

Conversation

@jtibshirani
Copy link
Contributor

Since the index may have more than one shard, we can't always predict how many
documents will fall in each slice. This PR simply removes the checks, since we
already test the slicing logic extensively in an integration test. The REST test
is now just a sanity check for the API.

Fixes #75212.

Since the index may have more than one shard, we can't always predict how many
documents will fall in each slice. This PR simply removes the checks, since we
already test the slicing logic extensively in an integration test. The REST test
is now just a sanity check for the API.
@jtibshirani jtibshirani added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.15.0 labels Jul 12, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 12, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@jtibshirani This LGTM, another alternative could be to explicitly define number of shards, but this also LGTM

@jtibshirani
Copy link
Contributor Author

Thanks @mayya-sharipova for reviewing. This felt like the best option to me, as I wanted to keep a variable number of shards for the other tests in this suite and we already check the slicing logic deeply in other tests.

@jtibshirani jtibshirani merged commit a2b8b53 into elastic:master Jul 14, 2021
@jtibshirani jtibshirani deleted the slice-query branch July 14, 2021 18:10
masseyke pushed a commit to masseyke/elasticsearch that referenced this pull request Jul 16, 2021
Since the index may have more than one shard, we can't always predict how many
documents will fall in each slice. This PR simply removes the checks, since we
already test the slicing logic extensively in an integration test. The REST test
is now just a sanity check for the API.

Fixes elastic#75212.
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 v7.15.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] ClientYamlTestSuiteIT test {yaml=search/350_point_in_time/point-in-time with slicing} failing

4 participants