Issue 34488:Remove Abstract Component by ricardojaferreira · Pull Request #35898 · elastic/elasticsearch

ricardojaferreira · 2018-11-26T09:13:32Z

TransportAction and BaseRestHandler now no longer extends AbstractComponent. The AbstractComponent no longer has usages so it was deleted.

Relates to #34488

elasticmachine · 2018-11-26T16:22:00Z

Pinging @elastic/es-core-infra

cbuescher

@ricardojaferreira I left two comments, also please note that this change in its current form doesn't compile and there are PRs that are still not merged that are necessary to go in before we are actually able to remove this class entirely (e.g. #35394)

cbuescher · 2018-11-26T16:29:22Z

server/src/main/java/org/elasticsearch/action/support/TransportAction.java

    protected final String actionName;
    private final ActionFilter[] filters;
    protected final TaskManager taskManager;
+    private static final Logger logger = LogManager.getLogger(TransportAction.class);


I'm not sure that we want every action extending this to use the same logger. Also the visivility won't work, this leaves a lot of compile errors for me in "server".

I think we should do the same thing here as we do for BaseRestHandler.

cbuescher · 2018-11-26T16:30:55Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java

        Setting.boolSetting("rest.action.multi.allow_explicit_index", true, Property.NodeScope);

    private final LongAdder usageCount = new LongAdder();
+    private static final Logger logger = LogManager.getLogger(BaseRestHandler.class);


This logger appears to be unused.

I was thinking of having something like:

@Deprecated protected static Logger logger = LogMangaer.getLogger(getClass());

in 6.x and master and then removing it entirely from master in a follow up change.

pgomulka · 2018-12-10T09:34:34Z

server/src/main/java/org/elasticsearch/action/support/TransportAction.java

    protected final String actionName;
    private final ActionFilter[] filters;
    protected final TaskManager taskManager;
+    @Deprecated


Why is this logger deprecated?

I think that subclasses of TransportAction should declare their own logger. But I think it'd be kinder that that were only a hard requirement for 7.0 so making a deprecated logger here is nice, so long as we follow up to zap it.

pgomulka

@ricardojaferreira great effort, thank you. I left some comments. Make sure the build passes to you locally. Definitely ./gradlew assemble precommit. Consider running ./gradlew check to see all tests passing

pgomulka · 2018-12-10T09:35:13Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java


    private final LongAdder usageCount = new LongAdder();
+    @Deprecated
+    protected static Logger logger = LogManager.getLogger(BaseRestHandler.class);


why is this logger deprecated?

should this be for getClass() instead of BaseRestHandler.class?

Yeah, getClass() I think.

Hi, Thanks for the comments. I am still understanding how the project works. Anyway, to use getClass() the logger cannot be static. Is that ok? (Or am I seeing something wrong here)

pgomulka · 2018-12-10T09:38:14Z

server/src/main/java/org/elasticsearch/action/support/TransportAction.java

 package org.elasticsearch.action.support;
-
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.message.ParameterizedMessage;


that seems to be unused, do we need that? (won't pass the build)

pgomulka · 2018-12-10T09:40:26Z

server/src/main/java/org/elasticsearch/action/support/TransportAction.java

    private final ActionFilter[] filters;
    protected final TaskManager taskManager;
+    @Deprecated
+    protected static Logger logger = LogManager.getLogger(TransportAction.class);


should this be for getClass() instead of TransportAction.class?

Same has above, use getClass with static?

cbuescher

Hi @ricardojaferreira, as far as I see your last commit only changed the visibility of the loggers in TransportAction and BaseRestHandler but their would still share the same logger with all inheriting classes. I left a comment along those lines. I might not be totally up to date with the current state of the final works on #34488 so I ask @nik9000 and @pgomulka to also take a look because they will know the remaining other work that needs to get done around it.

cbuescher · 2018-12-10T09:40:48Z

server/src/main/java/org/elasticsearch/action/support/TransportAction.java

    private final ActionFilter[] filters;
    protected final TaskManager taskManager;
+    @Deprecated
+    protected static Logger logger = LogManager.getLogger(TransportAction.class);


Using "LogManager.getLogger(TransportAction.class)" would still mean all inheriting classes using this would share the same logger. I don't think this is what we want. Something like "LogManager.getLogger(getClass())" might work for now. Also: why did you add the "@deprecated" annotation? The loggers themselves are not ment to be deprecated as far as I understand #34488 .

I like deprecating them because I think everything that wants a logger should declare one. That way we don't have a zillion loggers we don't use. I do think that if you deprecate something you should add javadoc explaining what to do instead. Something like:

/** * @deprecated declare your own logger. */ @Deprecated protected static Logger logger = LogManager.getLogger(TransportAction.class);

I will add the comment on deprecated loggers.

cbuescher · 2018-12-10T09:40:57Z

server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java


    private final LongAdder usageCount = new LongAdder();
+    @Deprecated
+    protected static Logger logger = LogManager.getLogger(BaseRestHandler.class);


same here as above.

cbuescher

Thanks for the update, LGTM

pgomulka · 2019-01-21T10:52:47Z

@ricardojaferreira could you please merge upstream master into this PR? (as it is failing backwards compatibility tests at the moment)
Or do you mind if I do it instead (still on your master).

pgomulka · 2019-01-24T15:10:48Z

@elasticmachine run packaging-sample

pgomulka · 2019-01-24T15:31:55Z

@elasticmachine run elasticsearch-ci/packaging-sample

pgomulka

LGTM. Build passes

pgomulka · 2019-01-25T07:36:00Z

@ricardojaferreira the PR now passed the build and I have merged your change. Thank you once again for you contribution!

Issue 34488:Remove Abstract Component

24c232a

ricardojaferreira mentioned this pull request Nov 26, 2018

Remove AbstractComponent from AbstractLifecycleComponent #35560

Merged

cbuescher added :Core/Infra/Core Core issues without another label >refactoring labels Nov 26, 2018

cbuescher added the v7.0.0 label Nov 26, 2018

cbuescher requested changes Nov 26, 2018

View reviewed changes

ricardojaferreira added 2 commits December 5, 2018 23:04

Merge remote-tracking branch 'upstream/master'

6d824ff

Issue 34488:Remove Abstract Component - Code review

c352bc8

pgomulka reviewed Dec 10, 2018

View reviewed changes

cbuescher requested changes Dec 10, 2018

View reviewed changes

Issue 34488:Using getClass and added comments to deprecated

600846d

cbuescher approved these changes Jan 17, 2019

View reviewed changes

Merge branch 'upmaster'

d79443d

cbuescher assigned pgomulka Jan 21, 2019

pgomulka added 2 commits January 23, 2019 16:04

Merge branch 'upmaster'

00a5346

Merge branch 'upmaster'

7326516

pgomulka approved these changes Jan 25, 2019

View reviewed changes

pgomulka merged commit df8fa97 into elastic:master Jan 25, 2019

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

ChrisHegarty unassigned pgomulka Oct 29, 2024

Conversation

ricardojaferreira commented Nov 26, 2018

Uh oh!

elasticmachine commented Nov 26, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgomulka commented Jan 24, 2019

Uh oh!

pgomulka left a comment

Choose a reason for hiding this comment

Uh oh!

pgomulka commented Jan 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pgomulka commented Jan 21, 2019 •

edited

Loading

pgomulka commented Jan 24, 2019 •

edited

Loading