From 99f5c8bfe7341ee14769cb0b4d9fa3791010cce9 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 31 Jul 2018 13:30:04 -0400 Subject: [PATCH 1/5] Core: Minor size reduction for AbstractComponent This removes a constructor from `AbstractComponent` and `AbstractLifecycleComponent` that we weren't using and it switches the logger creation away from one of the `Settings` flavored methods which are no longer needed. --- .../elasticsearch/common/component/AbstractComponent.java | 8 +------- .../common/component/AbstractLifecycleComponent.java | 4 ---- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java index 62d6e7e311d5d..07aea8a007ac1 100644 --- a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java +++ b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java @@ -34,13 +34,7 @@ public abstract class AbstractComponent { protected final Settings settings; public AbstractComponent(Settings settings) { - this.logger = Loggers.getLogger(getClass(), settings); - this.deprecationLogger = new DeprecationLogger(logger); - this.settings = settings; - } - - public AbstractComponent(Settings settings, Class customClass) { - this.logger = LogManager.getLogger(customClass); + this.logger = LogManager.getLogger(getClass()); this.deprecationLogger = new DeprecationLogger(logger); this.settings = settings; } diff --git a/server/src/main/java/org/elasticsearch/common/component/AbstractLifecycleComponent.java b/server/src/main/java/org/elasticsearch/common/component/AbstractLifecycleComponent.java index 3c4b35d5c3477..8a472954ab492 100644 --- a/server/src/main/java/org/elasticsearch/common/component/AbstractLifecycleComponent.java +++ b/server/src/main/java/org/elasticsearch/common/component/AbstractLifecycleComponent.java @@ -35,10 +35,6 @@ protected AbstractLifecycleComponent(Settings settings) { super(settings); } - protected AbstractLifecycleComponent(Settings settings, Class customClass) { - super(settings, customClass); - } - @Override public Lifecycle.State lifecycleState() { return this.lifecycle.state(); From 3a5ca43cf34108fe84d6604eb59c8fbe4b289a65 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 31 Jul 2018 14:39:07 -0400 Subject: [PATCH 2/5] Fix --- .../org/elasticsearch/common/component/AbstractComponent.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java index 07aea8a007ac1..167ffdb7bea25 100644 --- a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java +++ b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java @@ -23,7 +23,6 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Strings; import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.node.Node; From 226a5e8b5fe4f5c18d4ca0d1082fac6f79bf21ef Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 31 Jul 2018 17:25:44 -0400 Subject: [PATCH 3/5] Fix fun test thing --- .../common/component/AbstractComponent.java | 1 + .../elasticsearch/common/logging/ESLoggerFactory.java | 10 +++++++--- .../cluster/service/ClusterApplierServiceTests.java | 10 +++++----- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java index 167ffdb7bea25..07aea8a007ac1 100644 --- a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java +++ b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java @@ -23,6 +23,7 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Strings; import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.node.Node; diff --git a/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java b/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java index caa922b92cfd1..b45b55609f595 100644 --- a/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java +++ b/server/src/main/java/org/elasticsearch/common/logging/ESLoggerFactory.java @@ -38,9 +38,13 @@ public static Logger getLogger(String prefix, String name) { public static Logger getLogger(String prefix, Class clazz) { /* - * Do not use LogManager#getLogger(Class) as this now uses Class#getCanonicalName under the hood; as this returns null for local and - * anonymous classes, any place we create, for example, an abstract component defined as an anonymous class (e.g., in tests) will - * result in a logger with a null name which will blow up in a lookup inside of Log4j. + * At one point we didn't use LogManager.getLogger(clazz) because + * of a bug in log4j that has since been fixed: + * https://github.com/apache/logging-log4j2/commit/ae33698a1846a5e10684ec3e52a99223f06047af + * + * For now we continue to use LogManager.getLogger(clazz.getName()) + * because we expect to eventually migrate away from needing this + * method entirely. */ return getLogger(prefix, LogManager.getLogger(clazz.getName())); } diff --git a/server/src/test/java/org/elasticsearch/cluster/service/ClusterApplierServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/service/ClusterApplierServiceTests.java index f4c2090895b83..cbf8a7eda2b3e 100644 --- a/server/src/test/java/org/elasticsearch/cluster/service/ClusterApplierServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/service/ClusterApplierServiceTests.java @@ -118,13 +118,13 @@ public void testClusterStateUpdateLogging() throws Exception { mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test1", - clusterApplierService.getClass().getName(), + clusterApplierService.getClass().getCanonicalName(), Level.DEBUG, "*processing [test1]: took [1s] no change in cluster state")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test2", - clusterApplierService.getClass().getName(), + clusterApplierService.getClass().getCanonicalName(), Level.TRACE, "*failed to execute cluster state applier in [2s]*")); @@ -192,19 +192,19 @@ public void testLongClusterStateUpdateLogging() throws Exception { mockAppender.addExpectation( new MockLogAppender.UnseenEventExpectation( "test1 shouldn't see because setting is too low", - clusterApplierService.getClass().getName(), + clusterApplierService.getClass().getCanonicalName(), Level.WARN, "*cluster state applier task [test1] took [*] above the warn threshold of *")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test2", - clusterApplierService.getClass().getName(), + clusterApplierService.getClass().getCanonicalName(), Level.WARN, "*cluster state applier task [test2] took [32s] above the warn threshold of *")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test4", - clusterApplierService.getClass().getName(), + clusterApplierService.getClass().getCanonicalName(), Level.WARN, "*cluster state applier task [test3] took [34s] above the warn threshold of *")); From 5679be24cedafce5df9987472c4c98ba966fc10b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 31 Jul 2018 18:32:05 -0400 Subject: [PATCH 4/5] Checkstyle! --- .../org/elasticsearch/common/component/AbstractComponent.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java index 07aea8a007ac1..167ffdb7bea25 100644 --- a/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java +++ b/server/src/main/java/org/elasticsearch/common/component/AbstractComponent.java @@ -23,7 +23,6 @@ import org.apache.logging.log4j.Logger; import org.elasticsearch.common.Strings; import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.node.Node; From 18fac2f8929348f861bfb82f522c5fefe0ef6a31 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 31 Jul 2018 20:46:23 -0400 Subject: [PATCH 5/5] another test failure --- .../cluster/service/MasterServiceTests.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java index 1ef548bd68114..1168e1034fe6c 100644 --- a/server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/service/MasterServiceTests.java @@ -309,19 +309,19 @@ public void testClusterStateUpdateLogging() throws Exception { mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test1", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.DEBUG, "*processing [test1]: took [1s] no change in cluster state")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test2", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.TRACE, "*failed to execute cluster state update in [2s]*")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test3", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.DEBUG, "*processing [test3]: took [3s] done publishing updated cluster state (version: *, uuid: *)")); @@ -650,25 +650,25 @@ public void testLongClusterStateUpdateLogging() throws Exception { mockAppender.addExpectation( new MockLogAppender.UnseenEventExpectation( "test1 shouldn't see because setting is too low", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.WARN, "*cluster state update task [test1] took [*] above the warn threshold of *")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test2", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.WARN, "*cluster state update task [test2] took [32s] above the warn threshold of *")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test3", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.WARN, "*cluster state update task [test3] took [33s] above the warn threshold of *")); mockAppender.addExpectation( new MockLogAppender.SeenEventExpectation( "test4", - masterService.getClass().getName(), + masterService.getClass().getCanonicalName(), Level.WARN, "*cluster state update task [test4] took [34s] above the warn threshold of *"));