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.

Sign up for GitHub

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

Propogate version in reindex from remote search #42412

Merged
merged 11 commits into from
May 29, 2019
Merged

Propogate version in reindex from remote search #42412

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bade2ea
Reproduce
Tim-Brooks May 8, 2019
c196269
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 8, 2019
2498c41
Changes
Tim-Brooks May 9, 2019
e03c713
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 22, 2019
ad69803
Changes
Tim-Brooks May 22, 2019
52b86ff
Changes
Tim-Brooks May 22, 2019
18ae97d
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 22, 2019
0565718
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 23, 2019
855a709
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 24, 2019
ce3eee0
Merge remote-tracking branch 'upstream/master' into reindex_fix_external
Tim-Brooks May 29, 2019
623d415
Add condition
Tim-Brooks May 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/reindex/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ forbiddenPatterns {
exclude '**/*.p12'
}

// Support for testing reindex-from-remote against old Elaticsearch versions
// Support for testing reindex-from-remote against old Elasticsearch versions
configurations {
oldesFixture
es2
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to add a configuration for ES5 here now that we have ES 7?

Expand Down
13 changes: 6 additions & 7 deletions ...s/reindex/src/main/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuilders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,13 @@ static Request initialSearch(SearchRequest searchRequest, BytesReference query,
request.addParameter("scroll", keepAlive.getStringRep());
}
request.addParameter("size", Integer.toString(searchRequest.source().size()));
if (searchRequest.source().version() == null || searchRequest.source().version() == true) {
/*
* Passing `null` here just add the `version` request parameter
* without any value. This way of requesting the version works
* for all supported versions of Elasticsearch.
*/
request.addParameter("version", null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need the input from @nik9000 on this. In particular, it's unclear to me why we requested the version in case where the version was not set on searchRequest.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks as if the version parameter always existed, with the default being false (the parameter was added in 0.15, see #676). Also docs that go as far back as 0.90 seem to support this, see https://www.elastic.co/guide/en/elasticsearch/reference/0.90/search-request-version.html

I wonder if we should just write

request.addParameter("version", searchRequest.source().version());

here for simplicity. Would that break any tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addParameter does not take a boolean. It takes a string. Additionally, searchRequest.source().version() returns Boolean the object so it can be null. I think that if we want to treat null as false (which I think we do as our modern concept of null I think means false, but an old version of Elasticsearch will be true) we need to explicitly handle each of null, false, and true.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I have read the comments correctly, I think this is a tiny breaking change, in that using remote reindex against some old version (0.9 or 0.90?) would previously request the version, making it available to scripts. I wonder if we need to add a breaking change documentation for it?
I find the new way reasonable and I also think this can be backported to 7.x, unless anyone has objections to this?

Copy link
Contributor

@ywelsch ywelsch May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I find the new way reasonable and I also think this can be backported to 7.x, unless anyone has objections to this?

+1. I’m not aware of any scripts that would make use of the version field and reindex from 0.90 also becomes less and less common. I don't think there's a need for marking it as breaking or adding breaking-changes docs on this.


if (searchRequest.source().version() == null || searchRequest.source().version() == false) {
request.addParameter("version", Boolean.FALSE.toString());
} else {
request.addParameter("version", Boolean.TRUE.toString());
}

if (searchRequest.source().sorts() != null) {
boolean useScan = false;
// Detect if we should use search_type=scan rather than a sort
Expand Down
39 changes: 28 additions & 11 deletions modules/reindex/src/test/java/org/elasticsearch/index/reindex/ManyDocumentsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,35 @@ public void testReindexFromRemote() throws IOException {
Map<?, ?> http = (Map<?, ?>) nodeInfo.get("http");
String remote = "http://"+ http.get("publish_address");
Request request = new Request("POST", "/_reindex");
request.setJsonEntity(
if (randomBoolean()) {
request.setJsonEntity(
"{\n" +
" \"source\":{\n" +
" \"index\":\"test\",\n" +
" \"remote\":{\n" +
" \"host\":\"" + remote + "\"\n" +
" }\n" +
" }\n," +
" \"dest\":{\n" +
" \"index\":\"des\"\n" +
" }\n" +
"}");
" \"source\":{\n" +
" \"index\":\"test\",\n" +
" \"remote\":{\n" +
" \"host\":\"" + remote + "\"\n" +
" }\n" +
" }\n," +
" \"dest\":{\n" +
" \"index\":\"des\"\n" +
" }\n" +
"}");
} else {
// Test with external version_type
request.setJsonEntity(
"{\n" +
" \"source\":{\n" +
" \"index\":\"test\",\n" +
" \"remote\":{\n" +
" \"host\":\"" + remote + "\"\n" +
" }\n" +
" }\n," +
" \"dest\":{\n" +
" \"index\":\"des\",\n" +
" \"version_type\": \"external\"\n" +
" }\n" +
"}");
}
Map<String, Object> response = entityAsMap(client().performRequest(request));
assertThat(response, hasEntry("total", count));
assertThat(response, hasEntry("created", count));
Expand Down
43 changes: 31 additions & 12 deletions .../reindex/src/test/java/org/elasticsearch/index/reindex/remote/ReindexFromOldRemoteIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,38 @@ private void oldEsTestCase(String portPropertyName, String requestsPerSecond) th
}

Request reindex = new Request("POST", "/_reindex");
reindex.setJsonEntity(
if (randomBoolean()) {
// Reindex using the external version_type
reindex.setJsonEntity(
"{\n"
+ " \"source\":{\n"
+ " \"index\": \"test\",\n"
+ " \"size\": 1,\n"
+ " \"remote\": {\n"
+ " \"host\": \"http://127.0.0.1:" + oldEsPort + "\"\n"
+ " }\n"
+ " },\n"
+ " \"dest\": {\n"
+ " \"index\": \"test\"\n"
+ " }\n"
+ "}");
+ " \"source\":{\n"
+ " \"index\": \"test\",\n"
+ " \"size\": 1,\n"
+ " \"remote\": {\n"
+ " \"host\": \"http://127.0.0.1:" + oldEsPort + "\"\n"
+ " }\n"
+ " },\n"
+ " \"dest\": {\n"
+ " \"index\": \"test\",\n"
+ " \"version_type\": \"external\"\n"
+ " }\n"
+ "}");
} else {
// Reindex using the default internal version_type
reindex.setJsonEntity(
"{\n"
+ " \"source\":{\n"
+ " \"index\": \"test\",\n"
+ " \"size\": 1,\n"
+ " \"remote\": {\n"
+ " \"host\": \"http://127.0.0.1:" + oldEsPort + "\"\n"
+ " }\n"
+ " },\n"
+ " \"dest\": {\n"
+ " \"index\": \"test\"\n"
+ " }\n"
+ "}");
}
reindex.addParameter("refresh", "true");
reindex.addParameter("pretty", "true");
if (requestsPerSecond != null) {
Expand Down
23 changes: 13 additions & 10 deletions ...ndex/src/test/java/org/elasticsearch/index/reindex/remote/RemoteRequestBuildersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public void testInitialSearchParamsFields() {
// Test request without any fields
Version remoteVersion = Version.fromId(between(2000099, Version.CURRENT.id));
assertThat(initialSearch(searchRequest, query, remoteVersion).getParameters(),
not(either(hasKey("stored_fields")).or(hasKey("fields"))));
not(either(hasKey("stored_fields")).or(hasKey("fields"))));

// Test stored_fields for versions that support it
searchRequest = new SearchRequest().source(new SearchSourceBuilder());
Expand All @@ -157,14 +157,14 @@ public void testInitialSearchParamsFields() {
searchRequest.source().storedField("_source").storedField("_id");
remoteVersion = Version.fromId(between(0, 2000099 - 1));
assertThat(initialSearch(searchRequest, query, remoteVersion).getParameters(),
hasEntry("fields", "_source,_id,_parent,_routing,_ttl"));
hasEntry("fields", "_source,_id,_parent,_routing,_ttl"));

// But only versions before 1.0 force _source to be in the list
searchRequest = new SearchRequest().source(new SearchSourceBuilder());
searchRequest.source().storedField("_id");
remoteVersion = Version.fromId(between(1000099, 2000099 - 1));
assertThat(initialSearch(searchRequest, query, remoteVersion).getParameters(),
hasEntry("fields", "_id,_parent,_routing,_ttl"));
hasEntry("fields", "_id,_parent,_routing,_ttl"));
}

public void testInitialSearchParamsMisc() {
Expand All @@ -184,7 +184,7 @@ public void testInitialSearchParamsMisc() {
fetchVersion = randomBoolean();
searchRequest.source().version(fetchVersion);
}

Map<String, String> params = initialSearch(searchRequest, query, remoteVersion).getParameters();

if (scroll == null) {
Expand All @@ -193,7 +193,10 @@ public void testInitialSearchParamsMisc() {
assertScroll(remoteVersion, params, scroll);
}
assertThat(params, hasEntry("size", Integer.toString(size)));
assertThat(params, fetchVersion == null || fetchVersion == true ? hasEntry("version", null) : not(hasEntry("version", null)));
if (fetchVersion != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could verify that hasEntry("version", Boolean.FALSE.toString()) in the case where fetchVersion == null?

assertThat(params, fetchVersion ? hasEntry("version", Boolean.TRUE.toString()) :
hasEntry("version", Boolean.FALSE.toString()));
}
}

private void assertScroll(Version remoteVersion, Map<String, String> params, TimeValue requested) {
Expand All @@ -220,22 +223,22 @@ public void testInitialSearchEntity() throws IOException {
assertEquals(ContentType.APPLICATION_JSON.toString(), entity.getContentType().getValue());
if (remoteVersion.onOrAfter(Version.fromId(1000099))) {
assertEquals("{\"query\":" + query + ",\"_source\":true}",
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));
} else {
assertEquals("{\"query\":" + query + "}",
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));
}

// Source filtering is included if set up
searchRequest.source().fetchSource(new String[] {"in1", "in2"}, new String[] {"out"});
searchRequest.source().fetchSource(new String[]{"in1", "in2"}, new String[]{"out"});
entity = initialSearch(searchRequest, new BytesArray(query), remoteVersion).getEntity();
assertEquals(ContentType.APPLICATION_JSON.toString(), entity.getContentType().getValue());
assertEquals("{\"query\":" + query + ",\"_source\":{\"includes\":[\"in1\",\"in2\"],\"excludes\":[\"out\"]}}",
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));
Streams.copyToString(new InputStreamReader(entity.getContent(), StandardCharsets.UTF_8)));

// Invalid XContent fails
RuntimeException e = expectThrows(RuntimeException.class,
() -> initialSearch(searchRequest, new BytesArray("{}, \"trailing\": {}"), remoteVersion));
() -> initialSearch(searchRequest, new BytesArray("{}, \"trailing\": {}"), remoteVersion));
assertThat(e.getCause().getMessage(), containsString("Unexpected character (',' (code 44))"));
e = expectThrows(RuntimeException.class, () -> initialSearch(searchRequest, new BytesArray("{"), remoteVersion));
assertThat(e.getCause().getMessage(), containsString("Unexpected end-of-input"));
Expand Down