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

SourceExists HLRC uses GetSourceRequest instead of GetRequest #51789

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

timoninmaxim
Copy link
Contributor

Ref #50885
Link to the discussion

Source Exists API currently uses GetRequest that contains unsupported fields like stored_fields, version and version_type. So restrict the API to use GetSourceRequest and deprecates previous version.

Copy link

@drsantos20 drsantos20 left a comment

Choose a reason for hiding this comment

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

Hi @timoninmaxim i would suggest you to break your pr in small commits to help others to review your change

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks @timoninmaxim! Besides a small comment, this change looks good to me.

@@ -184,7 +180,7 @@ private static void doTestSourceExists(BiFunction<String, String, GetRequest> re
if (randomBoolean()) {
boolean realtime = randomBoolean();
getRequest.realtime(realtime);
if (realtime == false) {
if (!realtime) {
Copy link
Member

Choose a reason for hiding this comment

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

In the es code base we prefer use == false instead of !. The reason is that this is more visible when reading the code. A ! can easily be missed or forgotten to be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the clarification! fixed it.

@martijnvg
Copy link
Member

@elasticmachine test this please

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Java High Level REST Client)

@martijnvg martijnvg merged commit b7f113e into elastic:master Feb 5, 2020
@timoninmaxim timoninmaxim deleted the feature/source_exists_api branch February 5, 2020 08:03
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Feb 5, 2020
martijnvg added a commit that referenced this pull request Feb 5, 2020
#51789) (#51913)

Originates from #50885

Co-authored-by: Maxim <timonin.maksim@mail.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants