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

Remove legacy versions support for Low-Level RestClient Sniffer library #3087

Closed
tlfeng opened this issue Apr 27, 2022 · 4 comments · Fixed by #3487
Closed

Remove legacy versions support for Low-Level RestClient Sniffer library #3087

tlfeng opened this issue Apr 27, 2022 · 4 comments · Fixed by #3487
Assignees
Labels
bug Something isn't working Clients Clients within the Core repository such as High level Rest client and low level client v2.0.1 v3.0.0 Issues and PRs related to version 3.0.0

Comments

@tlfeng
Copy link
Collaborator

tlfeng commented Apr 27, 2022

Is your feature request related to a problem? Please describe.

Since OpenSearch version 2.0 comes out, there are code in Low-level REST Client Sniffer library to deal with Legacy ES version 2.x, which may cause inappropriate behavior when communicating with OpenSearch version 2.x:
https://github.com/opensearch-project/OpenSearch/blob/1.3.1/client/sniffer/src/main/java/org/opensearch/client/sniff/OpenSearchNodesSniffer.java#L262

Also in the tests, there are codes to test compatibility with Legacy version 2.x 5.x 6.x and 7.x:
https://github.com/opensearch-project/OpenSearch/blob/1.3.1/client/sniffer/src/test/java/org/opensearch/client/sniff/OpenSearchNodesSnifferParseTests.java#L88

RestClient Sniffer library:
It allows to automatically discover nodes from a running OpenSearch cluster and set them to an existing RestClient instance. It retrieves the nodes that belong to the cluster using the Nodes Info API (GET /_nodes/http) and uses jackson to parse the obtained json response. It's compatible with Legacy version 2.x and upwards.

Describe the solution you'd like
Do manual testing:

  1. Validate this is broken with OpenSearch version 2.x
  2. Validate it works with OpenSearch version 1.x

After validating:
In OpenSearch version 2.0:
At least remove the logic for Sniffer to be compatible with Legacy version 2.x.
In OpenSearch version 3.0:
Remove support to all legacy versions.

If necessary, add integration test to validate the actual behavior with a real cluster.

Describe alternatives you've considered

Additional context
Related to issue #2748 and #2773

@tlfeng tlfeng added enhancement Enhancement or improvement to existing feature or request untriaged v2.0.0 Version 2.0.0 Clients Clients within the Core repository such as High level Rest client and low level client v3.0.0 Issues and PRs related to version 3.0.0 and removed untriaged labels Apr 27, 2022
@tlfeng tlfeng changed the title Remove legacy versions support for Low Level RestClient Sniffer library Remove legacy versions support for Low-Level RestClient Sniffer library Apr 27, 2022
@kartg kartg removed the v2.0.0 Version 2.0.0 label Apr 28, 2022
@tlfeng tlfeng self-assigned this May 2, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented May 11, 2022

Created a Java project that built by Maven to test the behavior of the Sniffer library:

Main.java

import org.apache.http.HttpHost;
import org.opensearch.client.Node;
import org.opensearch.client.RestClient;
import org.opensearch.client.sniff.Sniffer;

import java.io.IOException;
import java.util.concurrent.TimeUnit;

public class Main {
    public static void main(String[] args) throws IOException, InterruptedException {
        RestClient restClient = RestClient.builder(new HttpHost("localhost", 9200, "http"))
                .build();
        Sniffer sniffer = Sniffer.builder(restClient).setSniffIntervalMillis(500).build();
        TimeUnit.MILLISECONDS.sleep(100);  // wait for sniffer update
        for (Node node: restClient.getNodes()) {
            System.out.println(node);
        }
        sniffer.close();
        restClient.close();
    }
}

The dependency in pom.xml

    <dependencies>
        <!-- https://mvnrepository.com/artifact/org.opensearch.client/opensearch-rest-client-sniffer -->
        <dependency>
            <groupId>org.opensearch.client</groupId>
            <artifactId>opensearch-rest-client-sniffer</artifactId>
            <version>1.0.0</version>
        </dependency>
    </dependencies>

Cluster setting:
node 1:

version: 1.1.0
node.name: node-1.1.0
http.port: 9200
(default node roles)

node 2:

version: 2.0.0-rc1
node.name: node-2.0.0
http.port: 9201
node.roles: ["data"]

node 3:

version: 1.3.2
node.name: node-1.3.2
http.port: 9202
node.roles: ["data"]

Running result of the Java program:

[host=http://127.0.0.1:9200, bound=[http://127.0.0.1:9200, http://[::1]:9200], name=node-1.1.0, version=1.1.0, roles=data,ingest,master,remote_cluster_client, attributes={}]
[host=http://127.0.0.1:9201, bound=[http://127.0.0.1:9201, http://[::1]:9201], name=node-2.0, version=2.0.0, roles=data,master, attributes={shard_indexing_pressure_enabled=[true]}]
[host=http://127.0.0.1:9202, bound=[http://[::1]:9202, http://127.0.0.1:9202], name=node-1.3.2, version=1.3.2, roles=data, attributes={shard_indexing_pressure_enabled=[true]}]

Actual node roles:

$ curl -X GET http://localhost:9200/_cat/nodes\?v 
ip        heap.percent ram.percent cpu load_1m load_5m load_15m node.role master name
127.0.0.1           18          10   0    0.30    0.17     0.20 d         -      node-1.3.2
127.0.0.1           17          10   0    0.30    0.17     0.20 d         -      node-2.0.0
127.0.0.1           18          10   0    0.30    0.17     0.20 dimr      *      node-1.1.0

@tlfeng
Copy link
Collaborator Author

tlfeng commented May 11, 2022

It shows that the node role is not properly retrieved for OpenSearch node of version 2.0.0, the "master" role is set into the properties of the node by mistake. The issue is validated.

@tlfeng
Copy link
Collaborator Author

tlfeng commented May 26, 2022

My following plan is to:

  1. Remove the support of old versions of Elasticsearch server, and make it support all OpenSearch versions correctly.
  2. Update unit test correspondingly.

About the unit test, I found there is a shell script to generate the response of Nodes Info API into a file, which will be used for testing the Sniffer parsing the nodes info.
I think the design is to manually run the script to update the test resources, whenever there is a breaking change in the response of Nodes Info API, to make the unit test accurate, instead of spinning up a real cluster in every test run.
I will update the script to make it suitable for OpenSearch.

@tlfeng tlfeng added bug Something isn't working and removed enhancement Enhancement or improvement to existing feature or request labels Jun 2, 2022
@tlfeng
Copy link
Collaborator Author

tlfeng commented Jun 6, 2022

PR #3487 resolves the issue, and the change has been backported into 2.x and 2.0 branches, by PR #3521 and #3522.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Clients Clients within the Core repository such as High level Rest client and low level client v2.0.1 v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants