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

Added master_timeout parameter to CAT segments #624

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

kolchfa-aws
Copy link
Collaborator

Fixes #486

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.

@kolchfa-aws kolchfa-aws added v2.0.0 v1.3.0 Issue: Any issues related to version v1.3.x 3 - Tech review PR: Tech review in progress labels Jun 1, 2022
@kolchfa-aws kolchfa-aws added this to the 2022-Q2 milestone Jun 1, 2022
@kolchfa-aws kolchfa-aws requested a review from a team as a code owner June 1, 2022 18:10
@kolchfa-aws kolchfa-aws self-assigned this Jun 1, 2022
Copy link
Contributor

@hdhalter hdhalter left a comment

Choose a reason for hiding this comment

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

Looks good!

@keithhc2
Copy link
Contributor

keithhc2 commented Jun 1, 2022

Should we have two versions of this? One for 1.3, which still uses master_timeout, and then another version for 2.0, which would be cluster_manager_timeout.

@kolchfa-aws
Copy link
Collaborator Author

So, it's the same parameter but it's named differently in 2.0? If so, I can create two different versions.

@keithhc2
Copy link
Contributor

keithhc2 commented Jun 1, 2022

So, it's the same parameter but it's named differently in 2.0? If so, I can create two different versions.

Yeah, I believe that's a breaking change introduced in 2.0. You can look at #513 to see the exact syntax for what cluster_manager should be.

@@ -46,6 +46,7 @@ In addition to the [common URL parameters]({{site.url}}{{site.baseurl}}/opensear
Parameter | Type | Description
:--- | :--- | :---
bytes | Byte size | Specify the units for byte size. For example, `7kb` or `6gb`. For more information, see [Supported units]({{site.url}}{{site.baseurl}}/opensearch/units/)..
master_timeout | Time | The amount of time to wait for a connection to the master node. Default is 30 seconds.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change master_timeout to cluster_manager_timeout, to fit our new nomenclature as described in #450

Copy link
Collaborator

Choose a reason for hiding this comment

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

And as Keith described, let's make a separate PR for 1.3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK thank you. Will do.

@kolchfa-aws kolchfa-aws removed the v1.3.0 Issue: Any issues related to version v1.3.x label Jun 2, 2022
_opensearch/rest-api/cat/cat-segments.md Outdated Show resolved Hide resolved
@kolchfa-aws kolchfa-aws requested a review from keithhc2 June 2, 2022 17:26
Copy link
Contributor

@keithhc2 keithhc2 left a comment

Choose a reason for hiding this comment

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

LGTM!

@kolchfa-aws kolchfa-aws merged commit b890c2e into main Jun 2, 2022
@Naarcha-AWS Naarcha-AWS deleted the Fix486-add-master-timeout-to-cat branch September 14, 2022 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Tech review PR: Tech review in progress v2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Request parameter 'master_timeout' is missing in the page of 'CAT Segments' API
4 participants