Skip to content

Conversation

@markharwood
Copy link
Contributor

@markharwood markharwood commented Jan 30, 2018

When false, will error if search either timeouts, has partial errors or has missing shards rather
than returning partial search results. A cluster-level setting provides a default for searches
with no flag.

Closes #27435

@markharwood
Copy link
Contributor Author

Initial commit here just for CI checking that forward-port of 6.x-based #27906 was successful. Once green I'll add a commit that changes the default for allowPartialSearchResults to false (meaning errors will be thrown by default when it looks like partial results would be returned)

@markharwood markharwood changed the title Add allow_partial_search_results flag to search requests with default setting true Add allow_partial_search_results flag to search requests with default setting false Jan 30, 2018
@markharwood markharwood requested a review from colings86 January 30, 2018 15:48
@markharwood markharwood added >feature :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Jan 30, 2018
@markharwood markharwood self-assigned this Jan 30, 2018
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

I left a couple of comments int eh tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is needed? This test doesn't appear to test partial results?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a necessity from a previously failing trial on 6.x - if I recall correctly it was for searches that rely on the use of field data. Ditto the other test

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is needed? This test doesn't appear to test partial results?

@markharwood
Copy link
Contributor Author

Reverted the code that defaults searches to strict (error on partial results). Will open a separate issue once this is pushed to consider changing the default to strict.

@markharwood
Copy link
Contributor Author

CI build failures look to all be related to BWC checks with the 6.3 snapshot that doesn't yet have the new allowPartialSearchResults flag serializing in SearchRequest

@jasontedor
Copy link
Member

If I can assist in integrating a change like this with cross-cutting BWC implications, please let me know. I am always happy to help.

@markharwood
Copy link
Contributor Author

@jasontedor Thanks for the offer of help. I have 2 PRs lined up ready to push (if I can get an OK on this one, @colings86 ?)
This PR is for master and is failing on BWC, but I have a seperate 6.x PR which is passing tests.
I thought the best way to sequence this might be to:

  1. push the 6.x PR which should be green but will then cause all master BWC search tests to fail (master will not de/serialize search requests to 6.x)
  2. push this master PR which should then hopefully be green

Not sure if there needs to be a gap between 1) and 2) waiting for the 6.x build to make it through the intake tests?

After all this is done I'll open a separate issue to open the debate on whether new cluster default setting is to fail on partial results.

Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM

@jasontedor
Copy link
Member

jasontedor commented Jan 31, 2018

@markharwood I think that this would help keep the build green every step of the way and let you integrate this at a leisurely pace:

  • push a commit to this PR switching all version-dependent serialization logic to Version.V_7_0_0
  • push a commit to this PR that disables BWC testing; make a change to the root build.gradle setting ext.bwc_tests_enabled to false in all projects.
  • at this point, you should be able to get a completely green build
  • merge this PR
  • prepare a backport of this change to 6.x, changing the version-dependent serialization logic to Version.V_6_3_0, and setting ext.bwc_tests_enabled to true
  • at this point, you should be able to get a completely green build on 6.x, and integrating this change there will have no impact on master
  • backport this PR with the above changes to 6.x
  • prepare a commit against master that changes the version-dependent serialization logic to Version.V_6_3_0 and reenables ext.bwc_tests_enabled; at this point you can run the BWC tests and they should be green
  • integrate the above change of the version logic and reenabling BWC into master

… setting = true.

When false, will error if search either timeouts, has partial errors or has missing shards rather
than returning partial search results. A cluster-level setting provides a default for searches
with no flag.

Closes elastic#27435
@markharwood markharwood merged commit 77d2dd2 into elastic:master Jan 31, 2018
@markharwood markharwood changed the title Add allow_partial_search_results flag to search requests with default setting false Add allow_partial_search_results flag to search requests with default setting true Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>feature :Search/Search Search-related issues that do not fall into other categories v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants