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

Rename cluster settings following new 2.x convention #1746

Merged
merged 1 commit into from
Apr 25, 2022

Conversation

peternied
Copy link
Member

@peternied peternied commented Apr 6, 2022

Description

Previously the setting was cluster.initial_master_nodes and has since
changed to cluster.initial_cluster_manager_nodes. This was causing
issues due to a bug in the fallback method that has been fixed in
OpenSearch, but getting ahead of this change before its needed.

Issues

See related opensearch-project/OpenSearch#2779

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

@cliu123
Copy link
Member

cliu123 commented Apr 6, 2022

The related issue blocking 2.0.0 has been fixed in OpenSearch core. This change can be a part of other inclusive renaming changes later in future release.

Previously the setting was `cluster.initial_master_nodes` and has since
changed to `cluster.initial_cluster_manager_nodes`.  This was causing
issues due to a bug in the fallback method that has been fixed in
OpenSearch, but getting ahead of this change before its needed.

See related opensearch-project/OpenSearch#2779

Signed-off-by: Peter Nied <petern@amazon.com>
@peternied peternied marked this pull request as ready for review April 8, 2022 18:06
@peternied peternied requested a review from a team April 8, 2022 18:06
@codecov-commenter
Copy link

Codecov Report

Merging #1746 (127a7fa) into main (54a920b) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1746      +/-   ##
============================================
- Coverage     60.42%   60.42%   -0.01%     
  Complexity     3197     3197              
============================================
  Files           253      253              
  Lines         18093    18092       -1     
  Branches       3245     3245              
============================================
- Hits          10933    10932       -1     
  Misses         5579     5579              
  Partials       1581     1581              
Impacted Files Coverage Δ
...ecurity/ssl/rest/SecuritySSLReloadCertsAction.java 84.78% <0.00%> (-0.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54a920b...127a7fa. Read the comment docs.

@davidlago
Copy link

Following this comment we might wan to reassess merging this PR for 2.0

@peternied
Copy link
Member Author

@davidlago Was this still up for consideration in the 2.0.0 release? As we are past the code freeze deadline it seems like this should be merged into the 2.X branch for the next release train.

@peternied
Copy link
Member Author

There is an issue in security plugin opensearch-project/security#1746 that we are considering to merge post code freeze. There has been a setting that was renamed as part of inclusive naming in opensearch core which is used in security plugin scripts.
Pros: New inclusive terms usage is in alignment within our release, its a focused change that is validated by CI checks before entering the distribution build. There is a minimal risk of destabilizing perf/stress tests in progress
Cons: Additional inclusive terminology changes are not in for security plugin, and has no outward facing impact making it a good candidate for the next patch release.

[Recapping offline conversation] While this change would be valuable, it does not meet the requirements for the 2.0.0-rc1 release. There are concerns about modifying the setup scripts late in the process. We will add this to the next release.

From opensearch-project/opensearch-build#1624 (comment)

@peternied
Copy link
Member Author

@opensearch-project/security Since we've diverged into a 2.0 branch, lets see about merging this pull request

@peternied peternied merged commit 0155939 into opensearch-project:main Apr 25, 2022
@peternied peternied deleted the initial-nodes branch April 25, 2022 16:09
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
…ect#1746)

Previously the setting was `cluster.initial_master_nodes` and has since
changed to `cluster.initial_cluster_manager_nodes`.  This was causing
issues due to a bug in the fallback method that has been fixed in
OpenSearch, but getting ahead of this change before its needed.

See related opensearch-project/OpenSearch#2779

Signed-off-by: Peter Nied <petern@amazon.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.

5 participants