Reindex-from-remote fails for external versions

[nik9000]

It looks to me like reindex-from-remote fails for version_type=external:

curl -XDELETE localhost:9200/test1
curl -XDELETE localhost:9200/test2

curl -HContent-Type:application/json -XPUT localhost:9200/test1 -d'{
  "mappings": {
    "document": {
      "properties": {
        "id": {
          "type": "keyword"
        },
        "field_str": {
          "type": "text"
        }
      }
    }
  }
}'

curl -HContent-Type:application/json -XPOST 'localhost:9200/test1/document/1?version=526&version_type=external&refresh=true' -d'{
  "id": 1,
  "field_str": "val1"
}'
curl -HContent-Type:application/json -XPOST 'localhost:9200/test1/document/2?version=527&version_type=external&refresh=true' -d'{
  "id": 2,
  "field_str": "val2"
}'

curl -HContent-Type:application/json -XPUT localhost:9200/test2 -d'{
  "mappings": {
    "document": {
      "properties": {
        "id": {
          "type": "keyword"
        },
        "field_str": {
          "type": "text"
        }
      }
    }
  }
}'

#Reindex using version_type external
curl -HContent-Type:application/json -XPOST localhost:9200/_reindex?pretty -d'{
  "conflicts": "proceed",
  "source": {
    "size": 1,
    "remote": {
      "host": "http://127.0.0.1:9200"
    },
    "index": "test1"
  },
  "dest": {
    "index": "test2",
    "version_type": "external"
  }
}'

It looks like we don't have any tests for reindex-from-remote and version_type=external which seems like a big whole.

It is also worth looking in to combining version_type=external with op_type. I don't recall that being a valid thing to do but I don't see anything stopping it. I'm not sure that it'll work properly though.

[elasticmachine]

Pinging @elastic/es-distributed

[SergSlipushenko]

@nik9000 Is this bug going to be fixed someday? It might be helpful at least to state in the docs the reindexing to a remote cluster is not supported for external versioning...

[Tim-Brooks]

I think I have identified why this is happening. In AbstractAsyncBulkByScrollAction we set that we need the version on SearchSourceBuilder. The RemoteScrollableHitSource calls RemoteRequestBuilders#initialSearch to convert this into the http request. In that method it does NOT propagate the need for the version:

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);
}

Unfortunately, if the version configured on SearchSourceBuilder is null, the version is not returned. Currently, the DefaultSearchContext.java is configured to not return the version by default:

private boolean version = false; // by default, we don't return versions

This leads to the exception, because the version (when not requested) is set to -1. And that is an invalid version.

I was able to reproduce this issue locally, if I modified the RemoteRequestBuilders#initialSearch to propagate "version"=true, the test started passing. However, I do not know if there are some backwards compatibility concerns? The comment in the RemoteRequestBuilders.java seems to suggest there might be. @nik9000 I think this was initially your work/comment. Do you have some context on that?

[nik9000]

This is reaching back pretty far in my memory but I'm fairly sure there are backwards compatibility problems with setting `version=true` in some versions of elasticsearch. We have those versions running for the reindex tests but we might not have that case covered. It's certainly worth running all the tests with the change locally to see. If it fails then I'd be comfortable with the coverage. If not then it'll take some investigating.

…

On Wed, May 15, 2019, 09:18 Tim Brooks ***@***.***> wrote: I think I have identified why this is happening. In AbstractAsyncBulkByScrollAction we set that we need the version on SearchSourceBuilder. The RemoteScrollableHitSource calls RemoteRequestBuilders#initialSearch to convert this into the http request. In that method it does NOT propagate the need for the version: 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); } Unfortunately, if the version configured on SearchSourceBuilder is null, the version is not returned. Currently, the DefaultSearchContext.java is configured to not return the version by default: private boolean version = false; // by default, we don't return versions This leads to the exception, because the version (when not requested) is set to -1. And that is an invalid version. I was able to reproduce this issue locally, if I modified the RemoteRequestBuilders#initialSearch to propagate "version"=true, the test started passing. However, I do not know if there are some backwards compatibility concerns? The comment in the RemoteRequestBuilders.java seems to suggest there might be. @nik9000 <https://github.com/nik9000> I think this was initially your work/comment. Do you have some context on that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31908?email_source=notifications&email_token=AABUXISRYS7RK5P3IM7UFHLPVQEQRA5CNFSM4FJB3NVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVOTY2Y#issuecomment-492649579>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABUXIX7PEFZRDS7KUUO76LPVQEQRANCNFSM4FJB3NVA> .