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

[Deprecation] Deprecate master_timeout in favor of cluster_manager_timeout #1788

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

manasvinibs
Copy link
Member

Signed-off-by: manasvinibs manasvis@amazon.com

Description

As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories.

In this change we are generating the json files for the _cat API 's specs by running following sample cmd:
node scripts/spec_to_console.js -g "../OpenSearch/rest-api-spec/src/main/resources/rest-api-spec/api/cat.cluster_manager.json" -d "src/plugins/console/server/lib/spec_definitions/json/generated"

Issues Resolved

#1685

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@manasvinibs manasvinibs requested a review from a team as a code owner June 23, 2022 20:39
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Looking good! Two things for further investigation:

  1. I'm suspicious of changes from "osd" to "kb"
  2. I want to make sure we understand/document the removal of "include_type_name" or {type} as a param. I'm guessing this was another change to the js client since the last time these were updated? But worth confirming and including in the commit info.

"osd",
"kb",
Copy link
Member

Choose a reason for hiding this comment

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

My guess is that these are regressions? Naively, I'd think osd is the correct value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This got replaced on running the spec generation script with latest OpenSearch "main" branch. Is there a way I can verify the correct value?

Copy link
Member

Choose a reason for hiding this comment

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

kb is the right value. When we forked and did renaming we just went ahead and just renamed a lot of references. This kb is related to bytes so it we seem we had an existing bug.

@@ -1,10 +1,10 @@
{
"indices.create": {
"url_params": {
"include_type_name": "__flag__",
Copy link
Member

Choose a reason for hiding this comment

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

this change may be fine, but seems unrelated to the other changes.

@manasvinibs manasvinibs force-pushed the Issue-1685 branch 2 times, most recently from fd64e78 to 9cd2e3d Compare June 23, 2022 23:06
@@ -5,7 +5,7 @@
"bytes": [
"b",
"k",
"osd",
Copy link
Member

@kavilla kavilla Jun 23, 2022

Choose a reason for hiding this comment

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

Ha, whoopsie this happened in our renaming when we turned kb to osd

@kavilla
Copy link
Member

kavilla commented Jun 23, 2022

I want to make sure we understand/document the removal of "include_type_name" or {type} as a param. I'm guessing this was another change to the js client since the last time these were updated? But worth confirming and including in the commit info.

I think this is a good change though. type was removed for 2.x. Perhaps we can close this issue as well with this PR:
#1290

…meout

As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories.

Issue - opensearch-project#1685
Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Sounds like all these changes are good - I'd just recommend also including the non-naming changes in the commit message just so it's obvious they were intentional.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

LGTM! Will mention what @joshuarrrr recommended on merge.

@kavilla kavilla merged commit c659831 into opensearch-project:main Jun 24, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 24, 2022
…meout (#1788)

As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories.

Includes updates from generation with the removal of type.

Issues:
#1685
#1290

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
(cherry picked from commit c659831)
kavilla pushed a commit that referenced this pull request Jun 24, 2022
…meout (#1788) (#1794)

As part of the meta issue opensearch-project/OpenSearch#2589 to track the plan and progress of applying inclusive naming across OpenSearch Repositories.

Includes updates from generation with the removal of type.

Issues:
#1685
#1290

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
(cherry picked from commit c659831)

Co-authored-by: Manasvini B Suryanarayana <manasvis@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Deprecation] Deprecate master_timeout in favor of cluster_manager_timeout
3 participants