Add remote info to the HLRC #49657

AntonShuvaev · 2019-11-27T17:01:49Z

The remote info call is a rest only API call, but should still be added
to the rest client. This PR adds it as well as relevant tests.

Ref #47678

elasticmachine · 2019-11-28T08:42:34Z

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

hub-cap · 2019-12-03T15:43:49Z

great work so far. Lets go one step further with it tho and copy out the request/response from server, and put it into the HLRC's module, so that we dont have to use/modify the server side code. This will all be generated one day so we want to keep any changes away from server. While it might be a bit tedious now, its something our team is working on doing (code generation, eventually).

This will require you writing some new tests (you can look at some of the other PRs that have been recently merged) for the request and response. The request is generally easy, and teh response is more work. Below are the tests you can inherit from and use as examples.

https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/AbstractResponseTestCase.java
https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/AbstractRequestTestCase.java

martijnvg · 2019-12-16T08:18:11Z

@J-Bean Thanks for adding this api to the HLRC! Can you copy the RemoteInfoRequest and RemoteInfoResponse to the hlrc module for the reasons @hub-cap mentioned? You can also remove any thing from these copies that the hlrc doesn't need (for example extending from ActionRequest and ActionResponse). Besides this this PR looks great!

AntonShuvaev · 2019-12-16T09:06:10Z

@martijnvg Sure, I'll fix all comments ASAP.

# Conflicts: # server/src/main/java/org/elasticsearch/transport/RemoteConnectionInfo.java

AntonShuvaev · 2019-12-18T09:15:55Z

@martijnvg I fixed all issues and updated the PR.

martijnvg

Thanks @J-Bean. I left another comment.

client/rest-high-level/src/test/java/org/elasticsearch/client/CCRIT.java

server/src/main/java/org/elasticsearch/transport/RemoteConnectionInfo.java

# Conflicts: # client/rest-high-level/src/test/java/org/elasticsearch/client/CCRIT.java # client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/CCRDocumentationIT.java # server/src/main/java/org/elasticsearch/transport/ProxyConnectionStrategy.java

AntonShuvaev · 2019-12-22T19:23:57Z

Made changes as per review comments.

martijnvg

Thanks for updating this pr! I left 2 small comments.

martijnvg · 2019-12-23T09:56:15Z

server/src/main/java/org/elasticsearch/action/admin/cluster/remote/RemoteInfoResponse.java

@@ -39,7 +39,7 @@
        infos = in.readList(RemoteConnectionInfo::new);
    }

-    RemoteInfoResponse(Collection<RemoteConnectionInfo> infos) {
+    public RemoteInfoResponse(Collection<RemoteConnectionInfo> infos) {


I think all changes to the server gradle module can be undone now?

It is used in RemoteInfoResponseTests.

Ah! I missed that.

.../rest-high-level/src/test/java/org/elasticsearch/client/cluster/RemoteInfoResponseTests.java

martijnvg · 2019-12-23T10:00:12Z

@elasticmachine test this please

martijnvg · 2019-12-24T07:40:37Z

@elasticmachine test this please

martijnvg · 2019-12-24T09:50:01Z

@J-Bean Thank you for your contribution. I will backport this now to the 7.x branch.

This reverts commit fa1a7c5.

martijnvg · 2019-12-24T10:42:38Z

@J-Bean I had to revert this PR due to a test failure. I will open a new pr that adds re-adds this API.

This reverts commit b7ac732.

Unreverts the commit that added the remote info api to HLRC (#49657). The additional change to the original PR, is that `org.elasticsearch.client.cluster.RemoteConnectionInfo` now parses the initial_connect_timeout field as a string instead of a TimeValue instance. The reason that this is needed is because that the initial_connect_timeout field in the remote connection api is serialized for human consumption, but not for parsing purposes. Therefore the HLRC can't parse it correctly (which caused test failures in CI, but not in the PR CI :( ). The way this field is serialized needs to be changed in the remote connection api, but that is a breaking change. We should wait making this change until rest api versioning is introduced. Co-Authored-By: j-bean anton.shuvaev91@gmail.com

The additional change to the original PR (elastic#49657), is that `org.elasticsearch.client.cluster.RemoteConnectionInfo` now parses the initial_connect_timeout field as a string instead of a TimeValue instance. The reason that this is needed is because that the initial_connect_timeout field in the remote connection api is serialized for human consumption, but not for parsing purposes. Therefore the HLRC can't parse it correctly (which caused test failures in CI, but not in the PR CI :( ). The way this field is serialized needs to be changed in the remote connection api, but that is a breaking change. We should wait making this change until rest api versioning is introduced. Co-Authored-By: j-bean <anton.shuvaev91@gmail.com>

The additional change to the original PR (#49657), is that `org.elasticsearch.client.cluster.RemoteConnectionInfo` now parses the initial_connect_timeout field as a string instead of a TimeValue instance. The reason that this is needed is because that the initial_connect_timeout field in the remote connection api is serialized for human consumption, but not for parsing purposes. Therefore the HLRC can't parse it correctly (which caused test failures in CI, but not in the PR CI :( ). The way this field is serialized needs to be changed in the remote connection api, but that is a breaking change. We should wait making this change until rest api versioning is introduced. Co-Authored-By: j-bean <anton.shuvaev91@gmail.com> Co-authored-by: j-bean <anton.shuvaev91@gmail.com>

Relates to elastic#47678

This reverts commit fa1a7c5.

Unreverts the commit that added the remote info api to HLRC (elastic#49657). The additional change to the original PR, is that `org.elasticsearch.client.cluster.RemoteConnectionInfo` now parses the initial_connect_timeout field as a string instead of a TimeValue instance. The reason that this is needed is because that the initial_connect_timeout field in the remote connection api is serialized for human consumption, but not for parsing purposes. Therefore the HLRC can't parse it correctly (which caused test failures in CI, but not in the PR CI :( ). The way this field is serialized needs to be changed in the remote connection api, but that is a breaking change. We should wait making this change until rest api versioning is introduced. Co-Authored-By: j-bean anton.shuvaev91@gmail.com

Add remote info to the HLRC

1cef15b

matriv added the :Core/Features/Java High Level REST Client label Nov 28, 2019

jakelandis requested a review from martijnvg December 12, 2019 15:17

AntonShuvaev added 2 commits December 17, 2019 16:00

Merge remote-tracking branch 'origin/master' into hlrc_remote_info

554ce53

# Conflicts: # server/src/main/java/org/elasticsearch/transport/RemoteConnectionInfo.java

Fix review issues

01101fe

martijnvg reviewed Dec 18, 2019

View reviewed changes

client/rest-high-level/src/test/java/org/elasticsearch/client/CCRIT.java Show resolved Hide resolved

server/src/main/java/org/elasticsearch/transport/RemoteConnectionInfo.java Outdated Show resolved Hide resolved

AntonShuvaev added 3 commits December 22, 2019 21:03

Fix after resolving conflicts

69b8f27

Copy classes from server to client

ff9a39f

martijnvg reviewed Dec 23, 2019

View reviewed changes

AntonShuvaev added 2 commits December 23, 2019 23:29

add license header

362880b

Merge remote-tracking branch 'origin/master' into hlrc_remote_info

cac126f

martijnvg added v7.6.0 v8.0.0 >enhancement labels Dec 24, 2019

martijnvg merged commit fa1a7c5 into elastic:master Dec 24, 2019

martijnvg added a commit that referenced this pull request Dec 24, 2019

Revert "Add remote info to the HLRC (#49657)"

9aa6066

This reverts commit fa1a7c5.

martijnvg added a commit that referenced this pull request Dec 24, 2019

Revert "Add remote info to the HLRC (#49657)"

b7ac732

This reverts commit fa1a7c5.

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Dec 24, 2019

Revert "Revert "Add remote info to the HLRC (elastic#49657)""

9038168

This reverts commit b7ac732.

martijnvg mentioned this pull request Dec 24, 2019

Hlrc add remote info api #50482

Merged

martijnvg added the backport pending label Dec 24, 2019

martijnvg mentioned this pull request Dec 24, 2019

Backport: add remote info to the HLRC #50483

Merged

AntonShuvaev deleted the hlrc_remote_info branch December 24, 2019 13:28

martijnvg removed the backport pending label Dec 24, 2019

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020

Add remote info to the HLRC (elastic#49657)

7c3c001

Relates to elastic#47678

SivagurunathanV pushed a commit to SivagurunathanV/elasticsearch that referenced this pull request Jan 23, 2020

Revert "Add remote info to the HLRC (elastic#49657)"

d911f18

This reverts commit fa1a7c5.

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote info to the HLRC #49657

Add remote info to the HLRC #49657

AntonShuvaev commented Nov 27, 2019

elasticmachine commented Nov 28, 2019

hub-cap commented Dec 3, 2019

martijnvg commented Dec 16, 2019

AntonShuvaev commented Dec 16, 2019

AntonShuvaev commented Dec 18, 2019

martijnvg left a comment

AntonShuvaev commented Dec 22, 2019

martijnvg left a comment

martijnvg Dec 23, 2019

AntonShuvaev Dec 23, 2019

martijnvg Dec 24, 2019

martijnvg commented Dec 23, 2019

martijnvg commented Dec 24, 2019

martijnvg commented Dec 24, 2019

martijnvg commented Dec 24, 2019

Add remote info to the HLRC #49657

Add remote info to the HLRC #49657

Conversation

AntonShuvaev commented Nov 27, 2019

elasticmachine commented Nov 28, 2019

hub-cap commented Dec 3, 2019

martijnvg commented Dec 16, 2019

AntonShuvaev commented Dec 16, 2019

AntonShuvaev commented Dec 18, 2019

martijnvg left a comment

Choose a reason for hiding this comment

AntonShuvaev commented Dec 22, 2019

martijnvg left a comment

Choose a reason for hiding this comment

martijnvg Dec 23, 2019

Choose a reason for hiding this comment

AntonShuvaev Dec 23, 2019

Choose a reason for hiding this comment

martijnvg Dec 24, 2019

Choose a reason for hiding this comment

martijnvg commented Dec 23, 2019

martijnvg commented Dec 24, 2019

martijnvg commented Dec 24, 2019

martijnvg commented Dec 24, 2019