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

Add Get Source API to the HLRC #50885

Merged
merged 5 commits into from
Jan 23, 2020

Conversation

timoninmaxim
Copy link
Contributor

Add Get Source API to the HLRC

relates: #47678

@elasticmachine
Copy link
Collaborator

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

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 for working on this! I left a comment around the request/response class.

* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response
*/
public RestResponse getSource(GetRequest getRequest, RequestOptions options) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

I think this api needs to use its own Request and Response class.

Although the get source api support almost all request parameters that the get api support, the get source api doesn't support stored_fields, version and version_type. The latter two may be supported in the future, but I don't see stored_fields ever be supported (that retrieves something that isn't part of the _source). Also I see that the get api may support doc_value_fields option to and this is unrelated to the _source like stored_fields is. Therefor I think that this api should have its own high level client side request class.

I also think that this api should have a dedicated response class, that just includes a Map<String, Object> field for the source instead of the generic RestResponse class.

Copy link
Contributor Author

@timoninmaxim timoninmaxim Jan 13, 2020

Choose a reason for hiding this comment

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

Server's RestGetSourceAction returns BytesRestResponse object. Actually HLRC getSouce method returns the same, I just used more generic class for the method signature. Will it be ok to replace RestResponse with BytesRestResponse, or Map<String, Object> is preferable anyway?

Copy link
Member

Choose a reason for hiding this comment

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

Server's RestGetSourceAction returns BytesRestResponse object.

So do many RestAction classes, it is a common way for rest actions to serialize the response to a byte array, so that the networking layer can send a response.

Will it be ok to replace RestResponse with BytesRestResponse, or Map<String, Object> is preferable anyway?

I think if the getSource(...) methods return a Map<String, Object> then that is ok too, because _source is the only thing that this API returns and the exists(...) and existsSource(...) methods in RestHighLevelClient class also return just a boolean.

Copy link
Contributor Author

@timoninmaxim timoninmaxim Jan 14, 2020

Choose a reason for hiding this comment

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

@martijnvg thanks for the clarification! Could I use new GetSourceRequest class with exists(...) method too as under the hood they use the same server's API? But it's already released with v6.6.0 (#34519), are we able break compatibility here?

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that in a separate PR. We can then deprecate the old methods in favour for the new methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've committed the requested change. Should I create an issue for change exists(...) or it's possible to link to this conversation in new PR?

Copy link
Member

Choose a reason for hiding this comment

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

No need to create an issue for this. You can just mention it in a new PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martijnvg Create PR for that #51789

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.

Left 2 small comments, otherwise LGTM.

return request;
}

static Request getSource(GetSourceRequest getSourceRequest) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a test for this in RequestConvertersTests?

import java.io.IOException;
import java.util.Map;

public final class GetSourceResponse {
Copy link
Member

Choose a reason for hiding this comment

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

There isn't much here, but maybe also add a test for this? You can extend from AbstractResponseTestCase, this should be straight forward.

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

@elasticmachine test this please

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!
When the build is successful then I will merge this.

@martijnvg
Copy link
Member

@elasticmachine test this please

@timoninmaxim
Copy link
Contributor Author

Hi @martijnvg ! I don't know why those tests are failed, locally they passed successfully. Is it possible to rerun the tests?

@martijnvg
Copy link
Member

Regarding hlrc module, this did fail:

16:32:41 > Task :client:rest-high-level:checkstyleTest FAILED
16:32:41 [ant:checkstyle] [ERROR] /dev/shm/elastic+elasticsearch+pull-request-1/client/rest-high-level/src/test/java/org/elasticsearch/client/core/GetSourceResponseTests.java:25: Using the '.*' form of import should be avoided - org.elasticsearch.common.xcontent.*. [AvoidStarImport]

If you run ./gradlew -p client/rest-high-level/ precommit or ./gradlew -p client/rest-high-level/ check (also runs all tests) then you should get the same failure as well. Can you replace the wildcard imports with the imports of the actual classes being used? If you push a fix for the checkstyle failure then I will re-run the build.

The elasticsearch-ci/default-distro ci failure is unrelated to your change and a rerun is ok in this case.

@timoninmaxim
Copy link
Contributor Author

It's strange. I've run client:rest-high-level [check] gradle command in Intellij IDEA and it was successful, but precommit is not. Thanks for clarification!

@martijnvg
Copy link
Member

@elasticmachine test this please

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.

I left a few comments around the GetSourceResponseTests test, which failed in CI.
I think that after this, this pr is good to get merged 🤞


@Override
protected SourceOnlyResponse createServerTestInstance(XContentType xContentType) {
BytesReference source = new BytesArray("{\"field\":\"value\"}");
Copy link
Member

Choose a reason for hiding this comment

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

This should be replaced by:

try (XContentBuilder sourceBuilder = XContentBuilder.builder(xContentType.xContent())) {
            sourceBuilder.startObject();
            sourceBuilder.field("field", "value");
            sourceBuilder.endObject();
            return new SourceOnlyResponse(BytesReference.bytes(sourceBuilder));
        } catch (IOException ioe) {
            throw new UncheckedIOException(ioe);
        }

Sometimes the test framework uses a different xcontent type, for example yaml or cbor and this is now hardcoded to json.

Copy link
Member

Choose a reason for hiding this comment

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

Actually this is not needed, because the XContentBuilder#rawValue invocation in the SourceOnlyResponse#toXContent() method does the right conversion.

public final class GetSourceResponseTests extends
AbstractResponseTestCase<GetSourceResponseTests.SourceOnlyResponse, GetSourceResponse> {

static class SourceOnlyResponse implements ToXContent {
Copy link
Member

Choose a reason for hiding this comment

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

You need to overwrite isFragment() method here and let it return false or implement ToXContentObject instead.
Otherwise the test base class tries to always add a json object, which is already added in this test.

Copy link
Contributor Author

@timoninmaxim timoninmaxim Jan 21, 2020

Choose a reason for hiding this comment

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

yes you're correct, I've just did it and send new commit

expected.put("field", "value");

assertThat(clientInstance.getSource(), equalTo(serverTestInstance.source));
assertThat(clientInstance.getSource(), equalTo(expected));
Copy link
Member

Choose a reason for hiding this comment

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

The body of this method can be replaced with: assertThat(clientInstance.getSource(), equalTo(Map.of("field", "value")));. Just checking for the expected map is sufficient here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did it too

@martijnvg
Copy link
Member

@elasticmachine test this please

@martijnvg
Copy link
Member

@elasticmachine run elasticsearch-ci/default-distro

@martijnvg martijnvg merged commit c019230 into elastic:master Jan 23, 2020
martijnvg pushed a commit to martijnvg/elasticsearch that referenced this pull request Jan 23, 2020
martijnvg added a commit that referenced this pull request Jan 23, 2020
Backport to 7.x branch of #50885.

Relates to #47678

Co-authored-by: Maxim <timonin.maksim@mail.ru>
@timoninmaxim timoninmaxim deleted the feature/get_source_api branch January 24, 2020 12:30
timoninmaxim added a commit to timoninmaxim/elasticsearch that referenced this pull request Feb 2, 2020
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.

5 participants