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

Adds inclusive naming #99

Closed

Conversation

harshavamsi
Copy link
Collaborator

Description

  • Renames master to cluster_manager
  • Adds cluster_manager_timeout while deprecating master_timeout
  • Adds a new cat/cluster_manager API while deprecating cat/master

Issues Resolved

Closes #72

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.

@harshavamsi harshavamsi requested review from a team and robsears as code owners September 9, 2022 02:50
@harshavamsi harshavamsi changed the title Inclusive naming Adds inclusive naming Sep 9, 2022
@saratvemulapalli
Copy link
Member

@tlfeng could you take a stab at this review?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Does this work equally well against multiple major versions of OpenSearch 1.x, 2.x and 3.x? If I am using an older version against OpenSearch 2.x can I upgrade to this client without rewriting any code? Then can I just start calling the new APIs to remove deprecations?

@dblock
Copy link
Member

dblock commented Sep 19, 2022

@Yury-Fridlyand you did this recently for .NET client - care to take a look at this?

@Yury-Fridlyand
Copy link

Yury-Fridlyand commented Sep 20, 2022

I'm noob in ruby, but I'll do my best.
My first finding so far:

# OpenSearch::API::Utils.__bulkify [
# { :index => { :_index => 'myindexA', :_type => 'mytype', :_id => '1', :data => { :title => 'Test' } } },
# { :update => { :_index => 'myindexB', :_type => 'mytype', :_id => '2', :data => { :doc => { :title => 'Update' } } } }
# ]
#
# # => {"index":{"_index":"myindexA","_type":"mytype","_id":"1"}}
# # => {"title":"Test"}
# # => {"update":{"_index":"myindexB","_type":"mytype","_id":"2"}}
# # => {"doc":{"title":"Update"}}

AFAIK, _type support was remoted in OpenSearch, see opensearch-project/OpenSearch#1940.
Please, check this comment, and it is outdated, update it and let me know. There are few more comments like this, perhaps they should be updated too.

dblock
dblock previously approved these changes Sep 30, 2022
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Want to make sure we're working on integration tests that run against both OpenSearch 1.x and 2.x.

Fix CI? Looks like the code needs to be server-version aware.

@harshavamsi
Copy link
Collaborator Author

Does this work equally well against multiple major versions of OpenSearch 1.x, 2.x and 3.x? If I am using an older version against OpenSearch 2.x can I upgrade to this client without rewriting any code? Then can I just start calling the new APIs to remove deprecations?

Yes, we've only deprecated the master nomenclature without removing them and added new endpoints for cluster_manager.

dblock
dblock previously approved these changes Oct 4, 2022
@wbeckler
Copy link

@nhtruong would you be able to review this? If you review, I'll approve.

nhtruong
nhtruong previously approved these changes Oct 20, 2022
@VachaShah
Copy link
Collaborator

@harshavamsi Can you rebase with main? This would need a CHANGELOG after the rebase.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
@harshavamsi harshavamsi force-pushed the inclusive_naming branch 2 times, most recently from aeb08b3 to 8edd88a Compare November 14, 2022 21:28
Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add support for inclusive naming
7 participants