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

Fix the support of RestClient Node Sniffer for version 2.x and update tests #3487

Merged
merged 6 commits into from
Jun 6, 2022
Merged

Fix the support of RestClient Node Sniffer for version 2.x and update tests #3487

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17a603f
Fix the support of RestClient Sniffer for OpenSearch 2.0 and update t…
May 26, 2022
97d0558
Remove a tab indent
Jun 2, 2022
b54ea87
Fix unit test failure and remove unused codes
Jun 2, 2022
612d930
Parse array value of node attributes setting
Jun 4, 2022
7dd9ea4
Update OpenSearchNodesSnifferTests to meet the array presentation for…
Jun 4, 2022
c6cfb2a
add whitespace in String.split() instead of using replceAll() to remo…
Jun 5, 2022
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
43 changes: 1 addition & 42 deletions client/sniffer/src/main/java/org/opensearch/client/sniff/OpenSearchNodesSniffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,53 +262,12 @@ private static Node readNode(String nodeId, JsonParser parser, Scheme scheme) th
realAttributes.put(entry.getKey(), singletonList(entry.getValue()));
}

if (version.startsWith("2.")) {
/*
* 2.x doesn't send roles, instead we try to read them from
* attributes.
*/
boolean clientAttribute = v2RoleAttributeValue(realAttributes, "client", false);
Boolean masterAttribute = v2RoleAttributeValue(realAttributes, "master", null);
Boolean dataAttribute = v2RoleAttributeValue(realAttributes, "data", null);
if ((masterAttribute == null && false == clientAttribute) || masterAttribute) {
roles.add("master");
}
if ((dataAttribute == null && false == clientAttribute) || dataAttribute) {
roles.add("data");
}
} else {
assert sawRoles : "didn't see roles for [" + nodeId + "]";
}
assert sawRoles : "didn't see roles for [" + nodeId + "]";
assert boundHosts.contains(publishedHost) : "[" + nodeId + "] doesn't make sense! publishedHost should be in boundHosts";
logger.trace("adding node [" + nodeId + "]");
return new Node(publishedHost, boundHosts, name, version, new Roles(roles), unmodifiableMap(realAttributes));
}

/**
* Returns {@code defaultValue} if the attribute didn't come back,
* {@code true} or {@code false} if it did come back as
* either of those, or throws an IOException if the attribute
* came back in a strange way.
*/
private static Boolean v2RoleAttributeValue(Map<String, List<String>> attributes, String name, Boolean defaultValue)
throws IOException {
List<String> valueList = attributes.remove(name);
if (valueList == null) {
return defaultValue;
}
if (valueList.size() != 1) {
throw new IOException("expected only a single attribute value for [" + name + "] but got " + valueList);
}
switch (valueList.get(0)) {
case "true":
return true;
case "false":
return false;
default:
throw new IOException("expected [" + name + "] to be either [true] or [false] but was [" + valueList.get(0) + "]");
}
}

/**
* The supported host schemes.
*/
Expand Down
90 changes: 26 additions & 64 deletions ...t/sniffer/src/test/java/org/opensearch/client/sniff/OpenSearchNodesSnifferParseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,59 +85,31 @@ private void checkFile(String file, Node... expected) throws IOException {
}
}

public void test2x() throws IOException {
checkFile(
"2.0.0_nodes_http.json",
node(9200, "m1", "2.0.0", true, false, false),
node(9201, "m2", "2.0.0", true, true, false),
node(9202, "m3", "2.0.0", true, false, false),
node(9203, "d1", "2.0.0", false, true, false),
node(9204, "d2", "2.0.0", false, true, false),
node(9205, "d3", "2.0.0", false, true, false),
node(9206, "c1", "2.0.0", false, false, false),
node(9207, "c2", "2.0.0", false, false, false)
);
}

public void test5x() throws IOException {
checkFile(
"5.0.0_nodes_http.json",
node(9200, "m1", "5.0.0", true, false, true),
node(9201, "m2", "5.0.0", true, true, true),
node(9202, "m3", "5.0.0", true, false, true),
node(9203, "d1", "5.0.0", false, true, true),
node(9204, "d2", "5.0.0", false, true, true),
node(9205, "d3", "5.0.0", false, true, true),
node(9206, "c1", "5.0.0", false, false, true),
node(9207, "c2", "5.0.0", false, false, true)
);
}

public void test6x() throws IOException {
public void test1x() throws IOException {
checkFile(
"6.0.0_nodes_http.json",
node(9200, "m1", "6.0.0", true, false, true),
node(9201, "m2", "6.0.0", true, true, true),
node(9202, "m3", "6.0.0", true, false, true),
node(9203, "d1", "6.0.0", false, true, true),
node(9204, "d2", "6.0.0", false, true, true),
node(9205, "d3", "6.0.0", false, true, true),
node(9206, "c1", "6.0.0", false, false, true),
node(9207, "c2", "6.0.0", false, false, true)
"1.0.0_nodes_http.json",
node(9200, "m1", "1.0.0", "master", "ingest"),
node(9201, "m2", "1.0.0", "master", "data", "ingest"),
node(9202, "m3", "1.0.0", "master", "ingest"),
node(9203, "d1", "1.0.0", "data", "ingest"),
node(9204, "d2", "1.0.0", "data", "ingest"),
node(9205, "d3", "1.0.0", "data", "ingest"),
node(9206, "c1", "1.0.0", "ingest"),
node(9207, "c2", "1.0.0", "ingest")
);
}

public void test7x() throws IOException {
public void test2x() throws IOException {
checkFile(
"7.3.0_nodes_http.json",
node(9200, "m1", "7.3.0", "master", "ingest"),
node(9201, "m2", "7.3.0", "master", "data", "ingest"),
node(9202, "m3", "7.3.0", "master", "ingest"),
node(9203, "d1", "7.3.0", "data", "ingest", "ml"),
node(9204, "d2", "7.3.0", "data", "ingest"),
node(9205, "d3", "7.3.0", "data", "ingest"),
node(9206, "c1", "7.3.0", "ingest"),
node(9207, "c2", "7.3.0", "ingest")
"2.0.0_nodes_http.json",
node(9200, "m1", "2.0.0", "cluster_manager", "ingest"),
node(9201, "m2", "2.0.0", "cluster_manager", "data", "ingest"),
node(9202, "m3", "2.0.0", "cluster_manager", "ingest"),
node(9203, "d1", "2.0.0", "data", "ingest"),
node(9204, "d2", "2.0.0", "data", "ingest"),
node(9205, "d3", "2.0.0", "data", "ingest"),
node(9206, "c1", "2.0.0", "ingest"),
node(9207, "c2", "2.0.0", "ingest")
);
}

Expand All @@ -163,33 +135,23 @@ public void testParsingPublishAddressWithES7Format() throws IOException {
assertEquals("http", nodes.get(0).getHost().getSchemeName());
}

private Node node(int port, String name, String version, boolean master, boolean data, boolean ingest) {
final Set<String> roles = new TreeSet<>();
if (master) {
roles.add("master");
}
if (data) {
roles.add("data");
}
if (ingest) {
roles.add("ingest");
}
return node(port, name, version, roles);
}

private Node node(int port, String name, String version, String... roles) {
return node(port, name, version, new TreeSet<>(Arrays.asList(roles)));
}

private Node node(int port, String name, String version, Set<String> roles) {
HttpHost host = new HttpHost("127.0.0.1", port);
Set<HttpHost> boundHosts = new HashSet<>(2);
boundHosts.add(host);
boundHosts.add(new HttpHost("[::1]", port));
boundHosts.add(host);
Map<String, List<String>> attributes = new HashMap<>();
attributes.put("dummy", singletonList("everyone_has_me"));
attributes.put("number", singletonList(name.substring(1)));
attributes.put("array", Arrays.asList(name.substring(0, 1), name.substring(1)));
attributes.put("array", singletonList(Arrays.asList(name.substring(0, 1), name.substring(1)).toString()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does look like we set now array as array of arrays (singletonList(Arrays.asList(...)), is it on purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @reta , thanks for the review! Let me explain it shortly, and I will seek for a better solution.

Copy link
Collaborator Author

@tlfeng tlfeng Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I probably should update the code of parsing the API response (such as [m, 1]) to be a Java array, instead of modifying the unit test. I will have a try.

It comes from a change in Elasticsearch 6.1, commit: elastic/elasticsearch@00dfdf5, doc: https://www.elastic.co/guide/en/elasticsearch/reference/6.1/release-notes-6.1.0.html#breaking-6.1.0

When adding the setting into yml config file:
node.attr.array: [m, 1]

There is a change of the notation of array value in the response of Nodes Info API:
In Elasticsearch 6.1 and above:

      "attributes": {
        "array": "[m, 1]"
      },

In Elasticsearch 6.0 and below:

      "attributes": {
        "array.0": "m",
        "array.1": "1"
      },

The current Sniffer code, can only parse the old format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I seem thank you for explaining @tlfeng

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the API response parser to meet the array presentation in ES 6.1 for the node attributes setting.

if (!version.startsWith("1.0") && !version.startsWith("1.1")) {
// Shard Indexing Pressure feature is added in version 1.2.0
attributes.put("shard_indexing_pressure_enabled", singletonList(Boolean.TRUE.toString()));
}
return new Node(host, boundHosts, name, version, new Roles(new TreeSet<>(roles)), attributes);
}

Expand Down
79 changes: 39 additions & 40 deletions .../src/test/resources/7.3.0_nodes_http.json → .../src/test/resources/1.0.0_nodes_http.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@
"transport_address": "127.0.0.1:9300",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"master",
"ingest"
"ingest",
"master"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "1",
"array.0": "m",
"array.1": "1"
"array": "[m, 1]"
},
"http": {
"bound_address": [
Expand All @@ -37,18 +37,18 @@
"transport_address": "127.0.0.1:9301",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"master",
"data",
"ingest"
"ingest",
"master"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "2",
"array.0": "m",
"array.1": "2"
"array": "[m, 2]"
},
"http": {
"bound_address": [
Expand All @@ -64,17 +64,17 @@
"transport_address": "127.0.0.1:9302",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"master",
"ingest"
"ingest",
"master"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "3",
"array.0": "m",
"array.1": "3"
"array": "[m, 3]"
},
"http": {
"bound_address": [
Expand All @@ -90,18 +90,17 @@
"transport_address": "127.0.0.1:9303",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"data",
"ingest",
"ml"
"ingest"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "1",
"array.0": "d",
"array.1": "1"
"array": "[d, 1]"
},
"http": {
"bound_address": [
Expand All @@ -117,17 +116,17 @@
"transport_address": "127.0.0.1:9304",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"data",
"ingest"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "2",
"array.0": "d",
"array.1": "2"
"array": "[d, 2]"
},
"http": {
"bound_address": [
Expand All @@ -143,17 +142,17 @@
"transport_address": "127.0.0.1:9305",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"data",
"ingest"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "3",
"array.0": "d",
"array.1": "3"
"array": "[d, 3]"
},
"http": {
"bound_address": [
Expand All @@ -169,16 +168,16 @@
"transport_address": "127.0.0.1:9306",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"ingest"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "1",
"array.0": "c",
"array.1": "1"
"array": "[c, 1]"
},
"http": {
"bound_address": [
Expand All @@ -194,16 +193,16 @@
"transport_address": "127.0.0.1:9307",
"host": "127.0.0.1",
"ip": "127.0.0.1",
"version": "7.3.0",
"build_hash": "8f0685b",
"version": "1.0.0",
"build_type": "tar",
"build_hash": "34550c5b17124ddc59458ef774f6b43a086522e3",
"roles": [
"ingest"
],
"attributes": {
"dummy": "everyone_has_me",
"number": "2",
"array.0": "c",
"array.1": "2"
"array": "[c, 2]"
},
"http": {
"bound_address": [
Expand Down
Loading