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 MultiSearchTemplate support to High Level Rest client #30836

Merged
merged 10 commits into from
Jun 28, 2018

Conversation

markharwood
Copy link
Contributor

Note some changes to core classes were required to support tests:

  • Added SearchType.currentlySupported to list only the usable options
  • Changed SearchResponse.Clusters constructor to public
  • Added SearchRequest.DEFAULT_BATCHED_REDUCE_SIZE constant
  • Added missing "tookInMillis" to MultiSearchTemplateResponse
  • Added SearchTemplateResponse toString() for comparisons in tests

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

Left a couple of comments, thanks @markharwood looks good. Can you also add docs please?

// Finally, serialize the deserialized request to compare JSON equivalence (in case Object.equals() fails to reveal a discrepancy)
String reserJsonRequestString = new String(MultiSearchTemplateRequest.writeMultiLineFormat(deser, XContentType.JSON.xContent()),
StandardCharsets.UTF_8);
assertEquals(jsonRequestString, reserJsonRequestString);
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 that the multiline format should be tested in elasticsearch given that the code belongs there? Something along the lines of MultiSearchRequestTests#testMultiLineSerialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that the multiline format should be tested in elasticsearch given that the code belongs there?

Perhaps not core - MultiSearchTemplateRequest is in a module because of the mustache dependency. Maybe a new class in /:modules:lang-mustache/src/test/java/org.elasticsearch.script.mustache ?

Copy link
Member

Choose a reason for hiding this comment

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

yea I mean where the request sits.

Copy link
Member

Choose a reason for hiding this comment

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

to clarify, I would leave the initial part and test here only the result of RequestConverters#multiSearchTemplate, and would move the multi line serialization roundtrips to another test to be placed where the request lives.

// The above notes the discrepancy between REST-configurable params and those in the Java client.
// It proposes the solution below:
setter.accept(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean(),
SearchRequest.DEFAULT_INDICES_OPTIONS));
Copy link
Member

Choose a reason for hiding this comment

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

good point, but I don't think that using the default options for search works for all the other API? At least not in theory. maybe we should allow to provide the default ones for each API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found this "options" part of the code pretty confusing to be honest. Could do with a quick chat about what's going on here.

Copy link
Member

Choose a reason for hiding this comment

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

IndicesOptions holds some options that can be set at REST (allow_no_indices, Ignore_unavailable, expand_wildcards) and some others that describe the behaviour of an API, which are hardcoded (on purpose). Unfortunately the transport client allows to set also the latter, with catastrophic results though :)

The point is that, for instance, the search API cannot work against closed indices, hence forbid closed indices is set to true in the default search api indices options, which means that closed indices are considered "unavailable". The default indices options matter as they hold default values, but also some values that cannot be overridden (unless you do it from the transport client at your own risk).

This is not a problem in the high-level REST client, as you can potentially set values like forbidClosedIndices etc. but they will be ignored, only the ones supported at REST will be converted to query_string params. That is what we check in these tests. I suspect that this change is needed because of your request comparison which should not be here but rather in the specific test around multiline serialization.

}

@Override
public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params params) throws IOException {
builder.startObject();
builder.field("took", tookInMillis);
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should have been added as part of #23767 . Can we make this change in a separate PR please? That way it will have a bit more visibility?

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 - that issue/PR will have to be pushed before this one because the fromXContent part of this PR was failing due to the missing tookInMillis property.


// Largely a copy of MultiSearchResponseTests - because multi template results are
// just wrapper objects for regular multi search results
public class MultiSearchTemplateResponseTests extends ESTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to extend from AbstractXContentTestCase?

Copy link
Contributor Author

@markharwood markharwood May 30, 2018

Choose a reason for hiding this comment

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

Tried this and not sure it would work - AbstractXContentTestCase tests for equality using toString() on the response object and any exceptions in the server-side instance don't compare exactly with the re-materialized exceptions from Xcontent. Likely this is why the MultiSearchResponseTests (from which I copied this test) does not use this base class.

Copy link
Member

Choose a reason for hiding this comment

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

you can override assertEqualInstances

Copy link
Member

Choose a reason for hiding this comment

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

MultiSearchResponseTests was written before the test base classes were made more flexible, we should probably migrate that too if it works well for your case

/**
* Non-deprecated types
*/
public static final SearchType [] CURRENTLY_SUPPORTED = {QUERY_THEN_FETCH, DFS_QUERY_THEN_FETCH};
Copy link
Member

Choose a reason for hiding this comment

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

curious about this: what problems were there with deprecated values? Given that it's only used in tests, maybe this could be only in the test where it's needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test was randomly picking from enum's values but the "fromString" code in the same enum would barf on one of those values.

Given that it's only used in tests, maybe this could be only in the test where it's needed?

I thought there would be less chance of getting it wrong in future if the enum where the values are decoded also listed the acceptable values it can decode.

Copy link
Member

Choose a reason for hiding this comment

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

ok no biggie

@markharwood
Copy link
Contributor Author

Happy with the latest changes, @javanna ?

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a couple more comments, mainly on testing, thanks @markharwood

@@ -2121,7 +2168,7 @@ private static void setRandomIndicesOptions(Consumer<IndicesOptions> setter, Sup
Map<String, String> expectedParams) {

if (randomBoolean()) {
setter.accept(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean()));
setter.accept(IndicesOptions.fromOptions(randomBoolean(), randomBoolean(), randomBoolean(), randomBoolean()));
Copy link
Member

Choose a reason for hiding this comment

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

restore the previous indentation? It was the correct one right? ;)

[[java-rest-high-multi-search-template-request]]
==== Multi-Search-Template Request

The `MultiSearchTemplateRequest` is built empty and you add all of the searches that
Copy link
Member

Choose a reason for hiding this comment

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

could you add the optional arguments section? I think max_concurrent_searches is the only one of them.


@Override
protected boolean supportsUnknownFields() {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

ideally, you would leave this to true and exclude some of the paths where injecting random fields is not supported, I can imagine that the top-level is problematic here and the _source part as well. You can override getRandomFieldsExcludeFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it and the debugger showed me it failing on an unknown random field name (something like "XCZXFDVC"). Is the end goal here to ensure strictness is being enforced (the test correctly rejects and reports any random noise in the xcontent fields) or that tolerance is applied (the rest client will silently tolerate unknown additions)?

Copy link
Member

Choose a reason for hiding this comment

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

that tolerance is applied, it's a way to test forward compatibility of the client.

Copy link
Member

Choose a reason for hiding this comment

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

obviously there are places where we injecting random fields makes no sense, like _source etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I don't think getRandomFieldsExcludeFilter will be enough and the parsing code in MultiSearchResponse.Item#itemFromXContent will need changing. Currently it has branching logic like this:

 if ("error".equals(fieldName)) {
	item = new Item(null, ElasticsearchException.failureFromXContent(parser));
} else  ...{
	// assume non-error and parse the hits etc
	item = new Item(SearchResponse.innerFromXContent(parser), null);

The getRandomFieldsExcludeFilter only lets me filter on the name but here I would need to filter if the object context being injected into already contains an error field as the code assumes that errors are the only field in an error response.


@Override
protected boolean assertToXContentEquivalence() {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

what made you return false here? checking whether this is expected or not. may have to do with exceptions which are parsed differently compared to their original representation. In that case, we could have an ordinary test without exceptions and a variation of it with exceptions where we disabled this check. We do this already in ListTasksResponseTests

@Override
protected boolean assertToXContentEquivalence() {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

same comments as above, thanks for converting this test though!!!


@Override
protected Predicate<String> getRandomFieldsExcludeFilter() {
return field -> field.startsWith("responses");
Copy link
Member

Choose a reason for hiding this comment

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

with the recent changes, I think that you only need the filter above when testing with failures. here and in the other test.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM besides the small comment I left on testing. thanks @markharwood !

@markharwood
Copy link
Contributor Author

run gradle build tests

Note some changes to core classes were required to support tests:

* Added SearchType.currentlySupported to list only the usable options
* Changed SearchResponse.Clusters constructor to public
* Added SearchRequest.DEFAULT_BATCHED_REDUCE_SIZE constant
* Added missing "tookInMillis" to MultiSearchTemplateResponse
* Added SearchTemplateResponse toString() for comparisons in tests
@markharwood markharwood merged commit 09dd19a into elastic:master Jun 28, 2018
@javanna javanna added the v6.4.0 label Jun 28, 2018
dnhatn added a commit that referenced this pull request Jun 28, 2018
* master:
  Docs: Remove duplicate test setup
  Print output when the name checker IT fails (#31660)
  Fix syntax errors in get-snapshots docs (#31656)
  Docs: Fix description of percentile ranks example example (#31652)
  Add MultiSearchTemplate support to High Level Rest client (#30836)
  Add test for low-level client round-robin behaviour (#31616)
  SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622)
  Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389)
  Correct integTest enable logic (#31646)
  Fix missing get-snapshots docs reference #31645
  Do not check for Azure container existence (#31617)
  Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580)
  Upgrade gradle wrapper to 4.8 (#31525)
  Only set vm.max_map_count if greater than default (#31512)
  Add Get Snapshots High Level REST API (#31537)
  QA: Merge query-builder-bwc to restart test (#30979)
  Update reindex.asciidoc (#31626)
  Docs: Skip xpack snippet tests if no xpack (#31619)
  mute CreateSnapshotRequestTests
  HLRest: Fix test for explain API
  [TEST] Fix RemoteClusterConnectionTests
  Add Create Snapshot to High-Level Rest Client (#31215)
  Remove legacy MetaDataStateFormat (#31603)
  Add explain API to high-level REST client (#31387)
  Preserve thread context when connecting to remote cluster (#31574)
  Unify headers for full text queries
  Remove redundant 'minimum_should_match'
  JDBC driver prepared statement set* methods  (#31494)
  [TEST] call yaml client close method from test suite (#31591)
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

4 participants