-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Replace 'discovered_master' with 'discovered_cluster_manager' in 'GET Cat Health' API #2438
Replace 'discovered_master' with 'discovered_cluster_manager' in 'GET Cat Health' API #2438
Conversation
Can one of the admins verify this patch? |
❌ Gradle Check failure 9eb177f1ff93a497f6ba9269969ca51b808ca1f7 |
…th api Signed-off-by: Tianli Feng <[email protected]>
9eb177f
to
c6ba67d
Compare
In log 3254:
It's not reproducible locally. |
start gradle check |
In log 3267:
Same test failure as issue #2440, but running on a different version of OpenSearch (v1.3.0). It can't be reproduced by the command |
Signed-off-by: Tianli Feng <[email protected]>
336231f
to
f9e741f
Compare
…ster_manager' for compatibility Signed-off-by: Tianli Feng <[email protected]>
f9e741f
to
49f5a9d
Compare
❌ Gradle Check failure 336231fd56670994df6337148c69066f4bb0655a |
In log 3306:
This is reproducible. The API response is 2.0.0 version, for a cluster with "minimum OpenSearch version [1.3.0], master version [1.3.0]".
|
✅ Gradle Check success f9e741ff6ee4d4952a54475fbb974b15a8a568f6 |
Signed-off-by: Tianli Feng <[email protected]>
In log 3312:
It's reported in issue #1746 |
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
21c4800
to
9486914
Compare
❌ Gradle Check failure 21c480000b050082dd3741119b9a3cbc270ba7d2 |
In log 3325:
|
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
In log 3333:
|
start gradle check |
@@ -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"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
dcm
as the alias for the table headerdiscovered_cluster_manager
discovered_master
as the alias for the table headerdiscovered_cluster_manager
, for keeping compatibility when usingGET _cat/nodes?v&h=discovered_master
to show the specific column only.Current response of 'GET Cat Health' API:
Proposed response of 'GET Cat Health' API:
Explanation on the API response compatibility
Because "cat API is a human-readable interface" (https://opensearch.org/docs/latest/opensearch/rest-api/cat/index/), and are not intended for use by software program, the old name "master" is not retained.
To keep the same API response with nodes in 1.x version, users can specify "header" in the request parameter, then the API response will remain the same for nodes in both 1.x and 2.x versions.
For example:
Testing
Issues Resolved
Part of #1549
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.