You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/09/28 20:43:56 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4237: ARTEMIS-4020 Using a little trick to create the Loggers
clebertsuconic opened a new pull request, #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237
Trick provided by Tim Bish
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983785094
##########
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/AbstractPrincipalLoginModule.java:
##########
@@ -31,12 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* Abstract login module that uses an external authenticated principal
*/
public abstract class AbstractPrincipalLoginModule implements AuditLoginModule {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
It's definitely no use to log.debug on an instance logger... that was a mistake... we had no defined workspace when this was done...
if this was doing log.info() with some information for the user I would agree with you.. .but log.debug() it's information for use on this case.... or someone debugging the code.
There's no point on doing log.debug on a instance logger on this case... we should remove them all.
We used to have others that I have historically replaced along the way.. it's just a mistake of stuff we used to do and it should be changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262527715
The precise implementation may have been a mistake, but there are other ways to acheive the same effect, which isnt unusual. The logging before was more useful than it will be with your change, where it will become less useful becuase you cant actually distinguish which of multiple things it is on behalf of.
You mentioned not being able to find where its logging (which per above is effectively what the change itsealf does). Its relatively trivial to follow a logger name of a specific subclass to a shared-implementation in its immediate parent. There are multiple mechanisms or login module classes that some of those shared-impl log statments could be for, and previously the logs would inherently identified exactly which one it was happening for. With your change they will instead all indicate the abstract parent, making it impossible to tell what one the logging was for if there happens to be more than one in use at the time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1261479657
@tabish121 I did not mean to change the tests under tests/activemq5-unit-tests... I meant to leave these alone...
I'm reverting my changes on that module... thanks for the catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983697391
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/scram/SCRAMServerSASLFactory.java:
##########
@@ -44,14 +44,16 @@
import org.apache.activemq.artemis.spi.core.security.scram.UserData;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* abstract class that implements the SASL-SCRAM authentication scheme, concrete implementations
* must supply the {@link SCRAM} type to use and be register via SPI
*/
public abstract class SCRAMServerSASLFactory implements ServerSASLFactory {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
I think it was an error to do it this way. When we look for log.debug, mixing instance versus class makes it fairly difficult to differentiate
I think we should keep it as static final class
think about it: you are looking over a log.debug(); the logger reports the wrong class name, and you can't find it...
it was definitely a mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262504325
I cant unresolve things but I dont consider the 'can no longer tell which mechanism failed' thing to be resolved. For that and the very similar 'which login module is this logging for', the change significantly reduces the utility of those log messages. They either need the log messages adjusted to actually report what child class they are logging on behalf of, or would be better to retain the existing behaviour, albeit implemented slightly differently...e.g by supplying the class or the logger from the child 'user' class so the parent can log on its behalf (e.g like Netty lets you do for its logging) rather than relying on the getClass() in the abstract parent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262512665
@gemmellr it was definitely a mistake to use an instanceLog on that case....
We should never use instanceLog for debugging...
for log.info stuff, we should always use a codified logging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262565648
It is almost certainly that way deliberately, and it makes much more sense that way from a 'look at logging, know what happened to what' perspective than it does with your proposed change. Unless you also change all the loggers accordingly to indicate what thing the message is being logged for, it would be far nicer to retain the current 'use logger name representing mechanism/module the logging is for' semantics. That could be better done by passing the class/logger into the parent than the preior getClass()-in-parent though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983645003
##########
tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpProtocolTracer.java:
##########
@@ -37,7 +39,7 @@
*/
public class AmqpProtocolTracer implements ProtocolTracer, FrameBodyHandler<AmqpFrameValidator> {
- private static final Logger TRACE_FRAMES = LoggerFactory.getLogger(AmqpProtocolTracer.class.getPackage().getName() + ".FRAMES");
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
I think we should keep the change here.. it's better this way.. as you said it's on the test client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983732721
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/scram/SCRAMServerSASLFactory.java:
##########
@@ -44,14 +44,16 @@
import org.apache.activemq.artemis.spi.core.security.scram.UserData;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* abstract class that implements the SASL-SCRAM authentication scheme, concrete implementations
* must supply the {@link SCRAM} type to use and be register via SPI
*/
public abstract class SCRAMServerSASLFactory implements ServerSASLFactory {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Its also a mistake to change it in this way.
Not all logging is debug level, the one here is a warning during exceptional failure, and if it ever occurs it wont be possible to distinguish what mechanism it was operating for when it failed after this change, as it will always log for the abstract parent. This is effectively breaking the original/useful aimed use of the logging. Either the logging statement also needs changed to indicate what its operating for, or it should pass in the mechanism-related logger to give the same effect it was doing originally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983630583
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultSSLContextFactory.java:
##########
@@ -15,18 +15,23 @@
*/
package org.apache.activemq.artemis.core.remoting.impl.ssl;
+import java.lang.invoke.MethodHandles;
import java.util.Map;
import javax.net.ssl.SSLContext;
import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextConfig;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextFactory;
import org.apache.activemq.artemis.utils.ConfigurationHelper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Simple SSLContextFactory for use in NettyConnector and NettyAcceptor.
*/
public class DefaultSSLContextFactory implements SSLContextFactory {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
as it should
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983641609
##########
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/AbstractPrincipalLoginModule.java:
##########
@@ -31,12 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* Abstract login module that uses an external authenticated principal
*/
public abstract class AbstractPrincipalLoginModule implements AuditLoginModule {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
We should make this standard... it was a mistake to do it like this.
Each class should have it's own final static logger.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983387224
##########
artemis-junit/src/main/java/org/apache/activemq/artemis/junit/EmbeddedActiveMQResource.java:
##########
@@ -66,11 +67,10 @@
* </code></pre>
*/
public class EmbeddedActiveMQResource extends ExternalResource {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Ditto
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultSSLContextFactory.java:
##########
@@ -15,18 +15,23 @@
*/
package org.apache.activemq.artemis.core.remoting.impl.ssl;
+import java.lang.invoke.MethodHandles;
import java.util.Map;
import javax.net.ssl.SSLContext;
import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextConfig;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextFactory;
import org.apache.activemq.artemis.utils.ConfigurationHelper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Simple SSLContextFactory for use in NettyConnector and NettyAcceptor.
*/
public class DefaultSSLContextFactory implements SSLContextFactory {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Ditto
##########
artemis-junit/src/main/java/org/apache/activemq/artemis/junit/EmbeddedJMSResource.java:
##########
@@ -78,6 +79,7 @@
*/
@Deprecated
public class EmbeddedJMSResource extends ExternalResource {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
This adds 'logger' but the class also has and is still using "log" below.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/sasl/scram/SCRAMServerSASLFactory.java:
##########
@@ -44,14 +44,16 @@
import org.apache.activemq.artemis.spi.core.security.scram.UserData;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* abstract class that implements the SASL-SCRAM authentication scheme, concrete implementations
* must supply the {@link SCRAM} type to use and be register via SPI
*/
public abstract class SCRAMServerSASLFactory implements ServerSASLFactory {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
This changes the name of the Logger, which is used for different mechanisms and passed around to related objects. The child classes should be changed to have their own static Logger and pass it via the constructor rather than creating here, to retain existing behaviour and allow distinguishing the mechanism the logging is actually for.
##########
tests/unit-tests/src/test/java/org/apache/activemq/artemis/tests/unit/logging/AssertionLoggerTest.java:
##########
@@ -64,7 +65,7 @@ public void testInfoAMQP() throws Exception {
private void validateLogging(Class<?> clazz) {
String randomLogging = RandomUtil.randomString();
- Logger logging = LoggerFactory.getLogger(clazz);
+ Logger logging = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
This breaks the intent of the method, it should be restored.
##########
artemis-junit/src/main/java/org/apache/activemq/artemis/junit/AbstractActiveMQClientResource.java:
##########
@@ -27,10 +27,11 @@
import org.junit.rules.ExternalResource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
public abstract class AbstractActiveMQClientResource extends ExternalResource {
- Logger log = LoggerFactory.getLogger(this.getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
This will change the logger name from being the specific non-abstract childs, to the abstract parent class.
May not be an issue, since the 'start' logging using it actually prints the class simple name, but just noting the difference.
Could pass in the logger to use to retain the existing naming behaviour.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -814,7 +815,7 @@ public void run() {
}
private static void checkCriticalAnalyzerLogging() {
- Logger criticalLogger = LoggerFactory.getLogger("org.apache.activemq.artemis.utils.critical");
+ Logger criticalLogger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
This changes the logger name used for the check to the wrong value, the one for ActiveMQServerImpl (and so also duplicates the class-level logger), rather than "org.apache.activemq.artemis.utils.critical" used before, which is what is also referenced in the log4j2.properties config.
Should be changed to a value derived from the declaration of the original logger elsewhere (maybe declared as a constant).
##########
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/AbstractPrincipalLoginModule.java:
##########
@@ -31,12 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* Abstract login module that uses an external authenticated principal
*/
public abstract class AbstractPrincipalLoginModule implements AuditLoginModule {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Similar to earlier comments, this changes the logger used by the subclasses to all be the same name, making it no longer possible to tell which one the logging related to. Should instead make them pass in their own static logger to retain existing logging behaviour.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/FileBasedNodeManager.java:
##########
@@ -32,14 +32,15 @@
import org.apache.activemq.artemis.utils.UUIDGenerator;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
import static java.nio.file.StandardOpenOption.CREATE;
import static java.nio.file.StandardOpenOption.READ;
import static java.nio.file.StandardOpenOption.WRITE;
public abstract class FileBasedNodeManager extends NodeManager {
- private static final Logger LOGGER = LoggerFactory.getLogger(FileBasedNodeManager.class);
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Should be deleted instead; name changed without any other uses changing, so its unused.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java:
##########
@@ -16,27 +16,32 @@
*/
package org.apache.activemq.artemis.core.remoting.impl.ssl;
+import java.lang.invoke.MethodHandles;
import java.util.Map;
import io.netty.handler.ssl.SslContext;
import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
*/
public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Since it is debug and really specific to this class, I think this is fine, especially as having a logger in an interface seems ugly, but just noting that this is changing the logger name used.
##########
artemis-server/src/test/java/org/apache/activemq/artemis/tests/util/ActiveMQTestBase.java:
##########
@@ -172,17 +173,17 @@
*/
public abstract class ActiveMQTestBase extends Assert {
- private static final Logger log = LoggerFactory.getLogger(ActiveMQTestBase.class);
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Might be worth looking at why there are 3 identical Logger declarations in this class (more below this one).
##########
tests/artemis-test-support/src/main/java/org/apache/activemq/transport/amqp/client/AmqpProtocolTracer.java:
##########
@@ -37,7 +39,7 @@
*/
public class AmqpProtocolTracer implements ProtocolTracer, FrameBodyHandler<AmqpFrameValidator> {
- private static final Logger TRACE_FRAMES = LoggerFactory.getLogger(AmqpProtocolTracer.class.getPackage().getName() + ".FRAMES");
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Noting it changes the logger name. Maybe not an issue as its only the test client.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983630235
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java:
##########
@@ -16,27 +16,32 @@
*/
package org.apache.activemq.artemis.core.remoting.impl.ssl;
+import java.lang.invoke.MethodHandles;
import java.util.Map;
import io.netty.handler.ssl.SslContext;
import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
import org.apache.activemq.artemis.spi.core.remoting.ssl.SSLContextConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
*/
public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
this is changing the logging name as it should. it was a mistake to have it like it was before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262540093
@gemmellr it's not meant to be that way... it was a mistake.. it needs to be fixed... that's all
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#issuecomment-1262699925
I don't really agree, but I will concede :) as I have spent more energy typing than reverting these changes :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r982874499
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/ActionAbstract.java:
##########
@@ -185,6 +185,7 @@ the ARTEMIS_HOME variable will include back slashes (An invalid file URI charact
@Override
public Object execute(ActionContext context) throws Exception {
+ System.out.println("I'm here");
Review Comment:
oops...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r983656021
##########
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/AbstractPrincipalLoginModule.java:
##########
@@ -31,12 +31,13 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
/**
* Abstract login module that uses an external authenticated principal
*/
public abstract class AbstractPrincipalLoginModule implements AuditLoginModule {
- private final Logger logger = LoggerFactory.getLogger(getClass());
+ private static final Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
Review Comment:
Basically all this is doing by using getClass() as the key is sharing a logger name (which various places in the codebase do with the code loggers, by using the package name as the key), so that all the logging for a given LoginModule all comes out the same logger name and can be reasoned about as a unit.
If you make this class have a static logger like this youll also need to change all the log messages themselves to better reflect what module they are logging about or you simply wont be able to tell which the logging is about unless there is only one. Passing a logger for such cases is not that unusual, it is done elsewhere in the codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] tabish121 commented on a diff in pull request #4237: ARTEMIS-4020 Using a little trick to create the Loggers
Posted by GitBox <gi...@apache.org>.
tabish121 commented on code in PR #4237:
URL: https://github.com/apache/activemq-artemis/pull/4237#discussion_r982860735
##########
tests/activemq5-unit-tests/src/test/java/org/apache/activemq/transport/failover/FailoverPriorityTest.java:
##########
@@ -37,6 +37,7 @@
import org.junit.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.lang.invoke.MethodHandles;
Review Comment:
This one doesn't seem like it needs the import, or it should actually use it in the logger declaration below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org