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

Cleanup network / transport related settings #25489

Merged
merged 4 commits into from
Jul 2, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Jun 30, 2017

This commit makes the use of the global network settings explicit instead
of implicit within NetworkService. It cleans up several places where we fall
back to the global settings while we should have used tcp or http ones.

In addition this change also removes unnecessary settings classes

This commit makes the use of the global network settings explicit instead
of implicit within NetworkService. It cleans up several places where we fall
back to the global settings while we should have used tcp or http ones.

In addition this change also removes unnecessary settings classes
@s1monw s1monw added :Core/Infra/Settings Settings infrastructure and APIs >enhancement v6.0.0 labels Jun 30, 2017
@s1monw s1monw requested review from rjernst and Tim-Brooks June 30, 2017 09:58
Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM too

if (addresses != null) {
return addresses;
}
// next check any registered custom resolvers if any
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think remove "next", since this is no longer a second step in the method

return addresses;
}
// next check any registered custom resolvers if any
if (customNameResolvers != null) {
Copy link
Member

@rjernst rjernst Jul 1, 2017

Choose a reason for hiding this comment

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

I don't think this can ever be null (it is initialized as an empty list when filling in Node), so you can add Objects.requireNonNull in the ctor and remove this check?

Copy link
Member

Choose a reason for hiding this comment

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

And ditto for this comment and the one above in the similar method below.

@s1monw s1monw merged commit 5a7c8bb into elastic:master Jul 2, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 4, 2017
* master: (52 commits)
  Include shared/attributes.asciidoc from docs master
  Fixed page breaks for ICU Collation Keyword Fields
  Remove QueryParseContext (elastic#25486)
  [Test] Use a common testing class for all XContent filtering tests (elastic#25491)
  Tests fix - Significant terms/text aggs  (elastic#25499)
  [DOCS] add docs for REST high level client index method (elastic#25501)
  Tests: Add Debian 9 (Stretch) to the packaging tests
  test: Run flush before upgrade and refresh after upgrade.
  Fix third party audit for repository-hdfs
  [TEST] Expect nodes getting disconnected quickly
  testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow
  Cleanup network / transport related settings (elastic#25489)
  Fix repository-hdfs plugin packaging test
  Remove allocation id from replica replication response (elastic#25488)
  Adjust BWC version on bad allocation request test
  Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497)
  Adjust status on bad allocation explain requests
  Preliminary support for ARM
  Add doc note regarding explicit publish host
  Fix typo in name of test
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants