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

Core: Remove settings from AbstractComponent #35140

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Oct 31, 2018

Stop passing Settings to AbstractComponent's ctor. This allows us to
stop passing around Settings in a ton of places. While this change
touches many files, it touches them all in fairly small, mechanical
ways, doing a few things per file:

  1. Drop the super(settings); line on everything that extends
    AbstractComponent.
  2. Drop the settings argument to the ctor if it is no longer used.
  3. If the file doesn't use logger then drop extends AbstractComponent from it.
  4. Clean up all compilation failure caused by the settings removal
    and drop any now unused settings isntances and method arguments.

I've intentionally not removed the settings argument from a few
files:

  1. TransportAction
  2. AbstractLifecycleComponent
  3. BaseRestHandler

These files don't need settings either, but this change is large
enough as is.

Relates to #34488

Stop passing `Settings` to `AbstractComponent`'s ctor. This allows us to
stop passing around `Settings` in a *ton* of places. While this change
touches many files, it touches them all in fairly small, mechanical
ways, doing a few things per file:
1. Drop the `super(settings);` line on everything that extends
`AbstractComponent`.
2. Drop the `settings` argument to the ctor if it is no longer used.
3. If the file doesn't use `logger` then drop `extends
AbstractComponent` from it.
4. Clean up all compilation failure caused by the `settings` removal
and drop any now unused `settings` isntances and method arguments.

I've intentionally *not* removed the `settings` argument from a few
files:
1. TransportAction
2. AbstractLifecycleComponent
3. BaseRestHandler

These files don't *need* `settings` either, but this change is large
enough as is.

Relates to elastic#34488
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@nik9000
Copy link
Member Author

nik9000 commented Oct 31, 2018

@jaymode this one is a bit of a monster and you've been so good about my previous ones on this path. Could you look it? I'm sorry in advance for the number of files. At least it isn't that many lines.

@nik9000 nik9000 changed the title Core: Less settings to AbstractComponent Core: Remove settings from AbstractComponent Oct 31, 2018
Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

I left one request, otherwise LGTM. @nik9000 thank you for keeping these digestible.

@@ -32,6 +32,9 @@

/**
* Test for EC2 network.host settings.
* <p>
* Warning: This test doesn't assert that the exceptions are thrown.
Copy link
Member

Choose a reason for hiding this comment

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

can you open up a issue for this test and assign it to someone? maybe the original author?

Copy link
Member Author

Choose a reason for hiding this comment

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


/**
* @deprecated declare your own logger
Copy link
Member

Choose a reason for hiding this comment

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

👍

@nik9000 nik9000 merged commit e28509f into elastic:master Nov 1, 2018
nik9000 added a commit that referenced this pull request Nov 2, 2018
Stop passing `Settings` to `AbstractComponent`'s ctor. This allows us to
stop passing around `Settings` in a *ton* of places. While this change
touches many files, it touches them all in fairly small, mechanical
ways, doing a few things per file:
1. Drop the `super(settings);` line on everything that extends
`AbstractComponent`.
2. Drop the `settings` argument to the ctor if it is no longer used.
3. If the file doesn't use `logger` then drop `extends
AbstractComponent` from it.
4. Clean up all compilation failure caused by the `settings` removal
and drop any now unused `settings` isntances and method arguments.

I've intentionally *not* removed the `settings` argument from a few
files:
1. TransportAction
2. AbstractLifecycleComponent
3. BaseRestHandler

These files don't *need* `settings` either, but this change is large
enough as is.

Relates to #34488
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 7, 2018
The previous merge introduced semantic conflicts that were not resolved at
merge time, particularly due to PRs elastic#35083, elastic#35140 and elastic#35172. This fixes that.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Nov 13, 2018
AbstractComponent was deprecated in elastic#35140 and is looking like it will be
removed at some point by elastic#34888. Today all it does is provide a logger. This
change removes the usages of AbstractComponent that live solely in the zen2
feature branch to avoid some future merge pain, and replaces it where necessary
with some directly-created loggers.
DaveCTurner added a commit that referenced this pull request Nov 13, 2018
AbstractComponent was deprecated in #35140 and is looking like it will be
removed at some point by #34888. Today all it does is provide a logger. This
change removes the usages of AbstractComponent that live solely in the zen2
feature branch to avoid some future merge pain, and replaces it where necessary
with some directly-created loggers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants