Skip to content

Rename InternalTestCluster#getMasterNodeInstance #83407

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

Conversation

DaveCTurner
Copy link
Contributor

This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use getCurrentMasterNodeInstance() instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.

This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use `getCurrentMasterNodeInstance()` instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure v8.1.0 labels Feb 2, 2022
@DaveCTurner DaveCTurner requested a review from fcofdez February 2, 2022 12:58
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Feb 2, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

Copy link
Contributor

@fcofdez fcofdez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@DaveCTurner
Copy link
Contributor Author

Going to backport this to 7.17 since it only touches test code and will help other backports.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 8, 2022
@elasticsearchmachine elasticsearchmachine merged commit 33b3cca into elastic:master Feb 8, 2022
@DaveCTurner DaveCTurner deleted the 2022-02-02-getMasterNodeInstance branch February 8, 2022 13:26
DaveCTurner added a commit that referenced this pull request Feb 8, 2022
This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use `getCurrentMasterNodeInstance()` instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.
DaveCTurner added a commit that referenced this pull request Feb 8, 2022
This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use `getCurrentMasterNodeInstance()` instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.
DaveCTurner added a commit that referenced this pull request Feb 8, 2022
This method's name is trappy: it is easy to misinterpret it as returning
an instance from the elected master, but in fact it uses any
master-eligible node. If you want an instance from the elected master,
you have to use `getCurrentMasterNodeInstance()` instead.

This commit renames the method to clarify that it might not get an
instance from the elected master, and adds docs with cross-refs to help
developers choose the right method.
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Feb 9, 2022
* upstream/master: (166 commits)
  Bind host all instead of just _site_ when needed (elastic#83145)
  [DOCS] Fix min/max agg snippets for histograms (elastic#83695)
  [DOCS] Add deprecation notice for system indices (elastic#83688)
  Cache ILM policy name on IndexMetadata (elastic#83603)
  [DOCS] Fix 8.0 breaking changes sort order (elastic#83685)
  [ML] fix random sampling background query consistency (elastic#83676)
  Move internal APIs into their own namespace '_internal'
  Runtime fields core-with-mapped tests support tsdb (elastic#83577)
  Optimize calculating the presence of a quorum (elastic#83638)
  Use switch expressions in EnableAllocationDecider and NodeShutdownAllocationDecider (elastic#83641)
  Note libffi error message in tmpdir docs (elastic#83662)
  Fix TransportDesiredNodesActionsIT batch tests (elastic#83659)
  [DOCS] Remove unused upgrade doc files (elastic#83617)
  [ML] Wait for model process to stop in stop deployment (elastic#83644)
  [ML] Fix submit after shutdown in process worker service (elastic#83645)
  Remove req/resp classes associated with HLRC (elastic#83599)
  Introduce index.version.compatibility setting (elastic#83264)
  Rename InternalTestCluster#getMasterNodeInstance (elastic#83407)
  Mute TimeSeriesIndexSearcherTests testCollectInOrderAcrossSegments (elastic#83648)
  Add rollover add max_primary_shard_docs condition (elastic#80981)
  ...

# Conflicts:
#	x-pack/plugin/rollup/build.gradle
#	x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v7.17.1 v8.0.0 v8.1.1 v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants