Entirely remove AbstractComponent #34488

nik9000 · 2018-10-15T20:12:56Z

AbstractComponent is trouble because its name implies that
everything should extend from it. It is useful, but maybe too
broadly useful. The things it offers access too, the Settings instance
for the entire server and a logger are nice to have around, but not
really needed everywhere.

So we should remove it and everything that needs a logger or a deprecation logger or a settings instance should just declare that it needs it. That way tests don't end up making a Settings instance "just because". That kind of thing makes test much harder to read.

See this comment for the part of this that is a "good first issue".

The text was updated successfully, but these errors were encountered:

elasticmachine · 2018-10-15T20:12:58Z

Pinging @elastic/es-core-infra

Drops the `deprecationLogger` from `AbstractComponent`, moving it to places where we need it. This saves us from building a bunch of `DeprecationLogger`s that we don't need. Relates to elastic#34488

Drops the `deprecationLogger` from `AbstractComponent`, moving it to places where we need it. This saves us from building a bunch of `DeprecationLogger`s that we don't need. 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

nik9000 · 2018-10-31T17:10:26Z

Once I merge #35140 I think this will be another one of those "good first issue", "help wanted" things where we folks can grab a couple of packages at the time and remove extends AbstractComponent and add a private static final Logger logger member.

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

nik9000 · 2018-11-01T01:39:17Z

OK! I've merged #34691 so a good chunk of this is now "good first issue" territory. In particular, replaceing extends AbstractComponent with private static final Logger logger = LogManager.getLogger(ThisClass.class);. I see 141 extends AbstractComponents so I figure this'd be good to break up. Something like:

server (except TransportAction, AbstractLifecycleComponent, BaseRestHandler) (@gavlyukovskiy) Remove use of AbstractComponent in server #35444
AbstractLifecycleComponent (@gwbrown) Remove AbstractComponent from AbstractLifecycleComponent #35560
x-pack (@jaymode) Remove use of AbstractComponent in xpack #35394
everywhere else (@jklancic) Removed AbstractComponent in remaining classes #35566

That way we don't get huge PRs. Which are not great for first PRs because they can have many merge conflicts.

nik9000 · 2018-11-01T01:44:47Z

I'd like to think a little more about TransportAction and BaseRestHandler. Those are very squarely in the plugin API and I wonder if in 6.x they should keep to protected final Logger logger = LogManager.getLogger(getClass()); on them. I've made so many breaking change for plugin developer in my quest to remove all of this ceremony I'm feeling a bit bad about it.

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates elastic#34488

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates #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 #34488

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates #34488

This commit removes the use of AbstractComponent in xpack where it was still being extended. It has been replaced with explicit logger declarations. See elastic#34488

nik9000 · 2018-11-14T18:05:54Z

Sure, I would love that @nik9000

I've added your name to my tracking comment. Enjoy! Thanks!

jklancic · 2018-11-15T02:01:07Z

I managed to get all remaining references removed @nik9000 . Here is the link to the PR. But I have an issue running the job ./gradlew check. I keep getting:

> Task :distribution:bwc:staged:buildBwcVersion FAILED

 [6.5.0] ERROR: JAVA_HOME is set to an invalid directory: null
 [6.5.0] 
 [6.5.0] Please set the JAVA_HOME variable in your environment to match the
 [6.5.0] location of your Java installation.
 [6.5.0] 

FAILURE: Build failed with an exception.

I manage to successfully run the test with ./gradlew test:

jernej@blackbook:~/Development/elasticsearch$ ./gradlew test

> Configure project :benchmarks
=======================================
Elasticsearch Build Hamster says Hello!
  Gradle Version        : 4.10
  OS Info               : Linux 4.15.0-39-generic (amd64)
  JDK Version           : 11 (Oracle Corporation 11.0.1 [Java HotSpot(TM) 64-Bit Server VM 11.0.1+13-LTS])
  JAVA_HOME             : /usr/lib/jvm/jdk-11.0.1
  Random Testing Seed   : 45BC1C408EE62CDA
=======================================

> Task :test:framework:test
==> Test Info: seed=71CEAE3B738261A9; jvms=4; suites=33
...
> Task :x-pack:plugin:ml:qa:no-bootstrap-tests:test
==> Test Info: seed=3B1DB42E203E751D; jvm=1; suite=1
==> Test Summary: 1 suite, 2 tests

Deprecated Gradle features were used in this build, making it incompatible with Gradle 5.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/4.10/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 25m 14s
579 actionable tasks: 294 executed, 285 up-to-date
jernej@blackbook:~/Development/elasticsearch$

I did not change anything inside the distribution modules. I only touched modules: plugins, test, server (there was a missing class). And I have the value set for JAVA_HOME:

jernej@blackbook:~/Development/elasticsearch$ echo $JAVA_HOME
/usr/lib/jvm/jdk-11.0.1

Here is also the configuration for :distribution:bwc:bugfix:buildBwcVersion:

> Task :distribution:bwc:bugfix:buildBwcVersion
 ...
 [6.4.4] 
 [6.4.4] > Configure project :benchmarks
 [6.4.4] =======================================
 [6.4.4] Elasticsearch Build Hamster says Hello!
 [6.4.4] =======================================
 [6.4.4]   Gradle Version        : 4.9
 [6.4.4]   OS Info               : Linux 4.15.0-39-generic (amd64)
 [6.4.4]   JDK Version (gradle)  : Oracle Corporation 10.0.2 [Java HotSpot(TM) 64-Bit Server VM 10.0.2+13]
 [6.4.4]   JAVA_HOME (gradle)    : /usr/lib/java/jdk-10.0.2
 [6.4.4]   JDK Version (compile) : Oracle Corporation 10.0.2 [Java HotSpot(TM) 64-Bit Server VM 10.0.2+13]
 [6.4.4]   JAVA_HOME (compile)   : /usr/lib/java/jdk-10.0.2
 [6.4.4]   JDK Version (runtime) : Oracle Corporation 1.8.0_191 [Java HotSpot(TM) 64-Bit Server VM 25.191-b12]
 [6.4.4]   JAVA_HOME (runtime)   : /usr/lib/jvm/jdk-8-191
 [6.4.4]   Random Testing Seed   : 8AB2C6297D1419D5

I set the environment variables in Linux in the file /etc/environment. The file contains the following values:

J2SDKDIR=/usr/lib/jvm/jdk-11.0.1
J2REDIR=/usr/lib/jvm/jdk-11.0.1
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/usr/lib/jvm/jdk-11.0.1/bin:/usr/lib/jvm/jdk-11.0.1/db/bin:"
JAVA_HOME=/usr/lib/jvm/jdk-11.0.1
DERBY_HOME=/usr/lib/jvm/jdk-11.0.1/db
JAVA8_HOME=/usr/lib/jvm/jdk-8-191
JAVA10_HOME=/usr/lib/java/jdk-10.0.2

The reason I point out the variables is, because something had a similar issue with Elasticsearch here.

Removes inhertiting from AbstractComponent for some classes (mostly in the plugins module). Relates to #34488

Removed extending of AbstractComponent and changed logger usage to explicit declaration. Abstract classes still have logger declaration using this.getClass() in order to show implementation class name in its logs. See #34488

nik9000 · 2018-11-27T16:54:07Z

@jkakavas, Task :distribution:bwc:staged:buildBwcVersion FAILED is usually caused by us bumping the version of Elasticsearch in an old branch. You'd need to merge master to get it.

This commit removes the use of AbstractComponent in xpack where it was still being extended. It has been replaced with explicit logger declarations. See #34488

siddu244 · 2018-12-06T07:53:34Z

Hey guys, iam new to open source
I am eager to contribute in elasticsearch, but facing issue in understanding the basic codebase of elastic search. Can someone provide me a documentaion of code base structure or help in understanding a basic code flow of elastic search it would be too helpful

jkakavas · 2018-12-06T10:17:38Z

@jkakavas, Task :distribution:bwc:staged:buildBwcVersion FAILED is usually caused by us bumping the version of Elasticsearch in an old branch. You'd need to merge master to get it.

ping @jklancic for visibility. (too many @jk* usernames in one issue )

nik9000 · 2018-12-06T17:03:06Z

Thanks @jkakavas! Sorry for that. I'm too quick on the keyboard sometimes. Every time I see github folks I ask them to implement nicer type-ahead features for @ in issues and PRs. It isn't an excuse, but it'd be nice!

@siddu244 I've actually just removed good first issue and help wanted from this issue as I found a victim on the Elasticsearch team and the remaining steps, while not hard, do touch a ton of code. As far as a general tour, I gave a presentation a while back that might be nice. But I expect it isn't as useful as finding something small and working on it. We don't have a current "big map" of the code base that I can point you to.

siddu244 · 2018-12-07T09:03:35Z

cool , let me check with presentation and get back if anything required. Thanks for the reply @jkakavas Best Regards... :)

…

On Thu, Dec 6, 2018 at 10:34 PM Nik Everett ***@***.***> wrote: Thanks @jkakavas <https://github.com/jkakavas>! Sorry for that. I'm too quick on the keyboard sometimes. Every time I see github folks I ask them to implement nicer type-ahead features for @ in issues and PRs. It isn't an excuse, but it'd be nice! @siddu244 <https://github.com/siddu244> I've actually just removed good first issue and help wanted from this issue as I found a victim on the Elasticsearch team and the remaining steps, while not hard, do touch a *ton* of code. As far as a general tour, I gave a presentation <https://www.elastic.co/elasticon/conf/2016/sf/contributing-to-elasticsearch-how-to-get-started> a while back that might be nice. But I expect it isn't as useful as finding something small and working on it. We don't have a current "big map" of the code base that I can point you to. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34488 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF50QBvyb34yMqyGOQC9zhKUMEzcFdtGks5u2U46gaJpZM4XdDvy> .

The AbstracLifecycleComponent used to extend AbstractComponent, so it had to pass settings to the constractor of its supper class. It no longer extends the AbstractComponent so there is no need for this constructor There is also no need for AbstracLifecycleComponent subclasses to have Settings in their constructors if they were only passing it over to super constructor. This is part 1. which will be backported to 6.x with a migration guide/deprecation log. part 2 will have this constructor removed in 7 relates #35560 relates #34488

…elastic#37488) The AbstracLifecycleComponent used to extend AbstractComponent, so it had to pass settings to the constractor of its supper class. It no longer extends the AbstractComponent so there is no need for this constructor There is also no need for AbstracLifecycleComponent subclasses to have Settings in their constructors if they were only passing it over to super constructor. This is part 1. which will be backported to 6.x with a migration guide/deprecation log. part 2 will have this constructor removed in 7 relates elastic#35560 relates elastic#34488

Adding the migration guide and removing the deprecated in 6.x constructor relates #35560 relates #34488

…88 (#37522) The AbstractLifecycleComponent used to extend AbstractComponent, so it had to pass settings to the constructor of its supper class. It no longer extends the AbstractComponent so there is no need for this constructor There is also no need for AbstractLifecycleComponent subclasses to have Settings in their constructors if they were only passing it over to super constructor. This is part 1. which will be backported to 6.x with a migration guide/deprecation log. part 2 will have this constructor removed in 7 relates #35560 relates #34488

nik9000 added :Core/Infra/Core Core issues without another label Meta >refactoring labels Oct 15, 2018

nik9000 mentioned this issue Oct 15, 2018

Core: Drop nodeName from AbstractComponent #34487

Merged

nik9000 mentioned this issue Oct 25, 2018

Drop deprecationLogger from AbstractComponent #34859

Merged

nik9000 mentioned this issue Oct 31, 2018

Core: Remove settings from AbstractComponent #35140

Merged

nik9000 mentioned this issue Oct 31, 2018

Replace another deprecated Loggers method with LogManager. #34691

Merged

nik9000 added good first issue low hanging fruit help wanted adoptme labels Nov 1, 2018

jaymode added a commit to jaymode/elasticsearch that referenced this issue Nov 1, 2018

Remove use of AbstractComponent in security

6ac2bd0

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates elastic#34488

jaymode mentioned this issue Nov 1, 2018

Remove use of AbstractComponent in security #35186

Merged

jaymode added a commit to jaymode/elasticsearch that referenced this issue Nov 1, 2018

Remove AbstractComponent usage in x-pack core

38ffdfb

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates elastic#34488

jaymode mentioned this issue Nov 1, 2018

Remove AbstractComponent usage in x-pack core #35187

Merged

jaymode added a commit that referenced this issue Nov 2, 2018

Remove use of AbstractComponent in security (#35186)

6b08d5f

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates #34488

jaymode added a commit that referenced this issue Nov 2, 2018

Remove AbstractComponent usage in x-pack core (#35187)

ec1c2c4

This change removes the use of AbstractComponent in the security module. The classes now declare their own loggers. Relates #34488

jaymode mentioned this issue Nov 8, 2018

Remove use of AbstractComponent in xpack #35394

Merged

gwbrown mentioned this issue Nov 14, 2018

Remove AbstractComponent from AbstractLifecycleComponent #35560

Merged

jklancic mentioned this issue Nov 16, 2018

Removed AbstractComponent in remaining classes #35566

Merged

cbuescher pushed a commit that referenced this issue Nov 16, 2018

Removes AbstractComponent from several classes (#35566)

baf33b3

Removes inhertiting from AbstractComponent for some classes (mostly in the plugins module). Relates to #34488

ricardojaferreira mentioned this issue Nov 26, 2018

Issue 34488:Remove Abstract Component #35898

Merged

jaymode added a commit that referenced this issue Nov 27, 2018

Remove use of AbstractComponent in xpack (#35394)

2061eeb

This commit removes the use of AbstractComponent in xpack where it was still being extended. It has been replaced with explicit logger declarations. See #34488

nik9000 removed good first issue low hanging fruit help wanted adoptme labels Dec 6, 2018

nik9000 assigned pgomulka Dec 6, 2018

pgomulka mentioned this issue Jan 15, 2019

Remove the use of AbstracLifecycleComponent constructor #37488

Merged

This was referenced Jan 16, 2019

Remove the use of AbstracLifecycleComponent constructor Backport#37488 #37522

Merged

Remove the AbstracLifecycleComponent constructor with Settings #37523

Merged

pgomulka added a commit that referenced this issue Jan 17, 2019

Remove the AbstracLifecycleComponent constructor with Settings (#37523)

b6e5cca

Adding the migration guide and removing the deprecated in 6.x constructor relates #35560 relates #34488

pgomulka added the v7.0.0 label Jan 25, 2019

pgomulka closed this as completed in df8fa97 Jan 25, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entirely remove AbstractComponent #34488

Entirely remove AbstractComponent #34488

nik9000 commented Oct 15, 2018 •

edited

Loading

elasticmachine commented Oct 15, 2018

nik9000 commented Oct 31, 2018

nik9000 commented Nov 1, 2018 •

edited by jaymode

Loading

nik9000 commented Nov 1, 2018

nik9000 commented Nov 14, 2018

jklancic commented Nov 15, 2018 •

edited

Loading

nik9000 commented Nov 27, 2018

siddu244 commented Dec 6, 2018

jkakavas commented Dec 6, 2018

nik9000 commented Dec 6, 2018

siddu244 commented Dec 7, 2018 via email

Entirely remove AbstractComponent #34488

Entirely remove AbstractComponent #34488

Comments

nik9000 commented Oct 15, 2018 • edited Loading

elasticmachine commented Oct 15, 2018

nik9000 commented Oct 31, 2018

nik9000 commented Nov 1, 2018 • edited by jaymode Loading

nik9000 commented Nov 1, 2018

nik9000 commented Nov 14, 2018

jklancic commented Nov 15, 2018 • edited Loading

nik9000 commented Nov 27, 2018

siddu244 commented Dec 6, 2018

jkakavas commented Dec 6, 2018

nik9000 commented Dec 6, 2018

siddu244 commented Dec 7, 2018 via email

nik9000 commented Oct 15, 2018 •

edited

Loading

nik9000 commented Nov 1, 2018 •

edited by jaymode

Loading

jklancic commented Nov 15, 2018 •

edited

Loading