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

Replace 'discovered_master' with 'discovered_cluster_manager' in 'GET Cat Health' API #2438

Merged
merged 9 commits into from
Mar 18, 2022
Merged

Replace 'discovered_master' with 'discovered_cluster_manager' in 'GET Cat Health' API #2438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c6ba67d
Replace discovered_master with discovered_cluster_manager in cat heal…
Mar 11, 2022
056f24f
Merge branch 'main' into replace-master-in-cat-health
Mar 13, 2022
49f5a9d
Add 'discovered_master' as the table header alias for 'discovered_clu…
Mar 13, 2022
9575e62
fix yaml rest rest by using node_selector to filter nodes in 2.x version
Mar 13, 2022
2d32ac0
Fix yaml rest test
Mar 14, 2022
eb3f1ed
Add comments in cat.health yaml rest test
Mar 14, 2022
9486914
Adjust format
Mar 14, 2022
8cfafc8
Merge branch 'main' into replace-master-in-cat-health
Mar 14, 2022
b89a754
Fix yaml rest test that changed by mistake
Mar 14, 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
42 changes: 39 additions & 3 deletions rest-api-spec/src/main/resources/rest-api-spec/test/cat.health/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,19 @@
---
"Help":
"Help - before 2.0.0":
- skip:
version: " - 7.10.99"
reason: "discovered_master added in OpenSearch 1.0.0"
version: " - 7.10.99 , 2.0.0 - "
reason: "discovered_master added in OpenSearch 1.0.0, and renamed to discovered_cluster_manager in 2.0.0"
features: node_selector
- do:
cat.health:
help: true
node_selector:
# Only send request to nodes in <2.0 versions, especially during ':qa:mixed-cluster:v1.x.x#mixedClusterTest'.
# Because YAML REST test takes the minimum OpenSearch version in the cluster to apply the filter in 'skip' section,
# see OpenSearchClientYamlSuiteTestCase#initAndResetContext() for detail.
# During 'mixedClusterTest', the cluster can be mixed with nodes in 1.x and 2.x versions,
# so node_selector is required, and only filtering version in 'skip' is not enough.
version: "1.0.0 - 1.4.99"

- match:
$body: |
Expand All @@ -27,6 +35,34 @@

$/

---
"Help":
- skip:
version: " - 1.4.99"
reason: "discovered_cluster_manager is added in OpenSearch 2.0.0"
- do:
cat.health:
help: true

- match:
$body: |
/^ epoch .+ \n
timestamp .+ \n
cluster .+ \n
status .+ \n
node.total .+ \n
node.data .+ \n
discovered_cluster_manager .+ \n
shards .+ \n
pri .+ \n
relo .+ \n
init .+ \n
unassign .+ \n
pending_tasks .+ \n
max_task_wait_time .+ \n
active_shards_percent .+ \n

$/

---
"Empty cluster":
Expand Down
7 changes: 6 additions & 1 deletion server/src/main/java/org/opensearch/rest/action/cat/RestHealthAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,12 @@ protected Table getTableWithHeader(final RestRequest request) {
t.addCell("status", "alias:st;desc:health status");
t.addCell("node.total", "alias:nt,nodeTotal;text-align:right;desc:total number of nodes");
t.addCell("node.data", "alias:nd,nodeData;text-align:right;desc:number of nodes that can store data");
t.addCell("discovered_master", "alias:dm;text-align:right;desc:discovered master");
Copy link
Member

Choose a reason for hiding this comment

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

With the Cluster Health API - #2437 - we chose to add a "cluster_manager" field alongside the "master" one, but here we're replacing "master" with "cluster_manager". Why the difference in approach?

Copy link
Member

Choose a reason for hiding this comment

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

D'oh, should've read the PR description more carefully 😅

To answer my own question - #2437 is adding a new field in the response, so we cannot replace it in-line. In this PR, the change affects the human-readable table header in the output, which we hardly expect to be parsed by code.

Feel free to ignore my question above 😄

Copy link
Collaborator Author

@tlfeng tlfeng Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @kartg, I really appreciate your careful attitude, to put the explanation in detail.
It's also my negligence that I didn't explain more in this PR description, after add more description in anther PR with the same concern: #2441 (comment) .
I will update in this PR description to encourage users who wants to use the same API request to parse the response of CAT Health in 2.x nodes, to filter output by specifying table header, such as GET _cat/health?v&h=discovered_master

// TODO: Remove the header alias 'discovered_master', after removing MASTER_ROLE.
// The alias 'discovered_master' is added for compatibility when using request parameter 'h=discovered_master'.
t.addCell(
"discovered_cluster_manager",
"alias:dcm,dm,discovered_master;text-align:right;desc:cluster manager is discovered or not"
);
t.addCell("shards", "alias:t,sh,shards.total,shardsTotal;text-align:right;desc:total number of shards");
t.addCell("pri", "alias:p,shards.primary,shardsPrimary;text-align:right;desc:number of primary shards");
t.addCell("relo", "alias:r,shards.relocating,shardsRelocating;text-align:right;desc:number of relocating nodes");
Expand Down