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 13:54:53 UTC
[GitHub] [activemq-artemis] gemmellr opened a new pull request, #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
gemmellr opened a new pull request, #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235
Switch to using SLF4J API for logging, use Log4J 2 as impl for the broker distribution and tests.
PR includes work from myself and Clebert Suconic, squashed from the new-logging branch.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261218357
@gemmellr @tabish121 I would merge this PR right away... any further improvements can be done after its merge.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982582723
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
I would not do that as part of this PR. otherwise it would never get done.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261132923
@gemmellr, @tabish121 would the trick apply to static instances? I would prefer to keep the loggers are static final.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982576669
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
Yeah there are plenty of concats left..I converted a bunch when I happened to be changing a specific line already, or just had for others in a class, but definitely we didnt try to remove them all (Clebert even likes them :P). A whole other massive PR could be done around that and e.g removing various gating still existing.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261112526
> Possible future enhancement would be to replace hard coding class names in logger definitions with Methodhandle lookups
>
> `Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());`
>
> This tends to help when cutting and pasting loggers and forgetting to change the class name.
Yeah I didnt know about that trick, probably would be good to consider as I knew there were a few incorrect ones (forget it they are fixed in here, or were previously on main).
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982602873
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
Indeed the changes would be large given how many bad logging usages I see. Most of the gating that is currently done is either not needed or wouldn't be needed if loggers used the formatting style as they 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] tabish121 commented on pull request #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261130569
> > Possible future enhancement would be to replace hard coding class names in logger definitions with Methodhandle lookups
> > `Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());`
> > This tends to help when cutting and pasting loggers and forgetting to change the class name.
>
> Yeah I didnt know about that trick, probably would be good to consider as I knew there were a few incorrect ones (forget it they are fixed in here, or were previously on main). But yes, I'd say better to do that separately later.
Indeed the changes would be large given how many bad logging usages I see. Most of the gating that is currently done is either not needed or wouldn't be needed if loggers used the formatting style as they 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] tabish121 commented on pull request #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261136829
> @gemmellr, @tabish121 would the trick apply to static instances? I would prefer to keep the loggers are static final.
Of course, it'd be:
`private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());`
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982543978
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
Unnecessary string concatenation could be avoided here.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982604558
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
> I'll go so far as to say I will work up such an improvement PR, but I'd like to keep it separate and land this one to avoid rebasing the changes yet more as other dev continues
Happy to help out if you need it.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#discussion_r982602673
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/utils/AutoCreateUtil.java:
##########
@@ -47,7 +50,7 @@ public static void autoCreateQueue(ClientSession session, SimpleString destAddr
.setAddress(destAddress);
setRequiredQueueConfigurationIfNotSet(queueConfiguration,response, RoutingType.ANYCAST, selectorString, true);
session.createQueue(queueConfiguration);
- ActiveMQClientLogger.LOGGER.debug("The queue " + destAddress + " was created automatically");
+ logger.debug("The queue " + destAddress + " was created automatically");
Review Comment:
I'll go so far as to say I will work up such an improvement PR, but I'd like to keep it separate and land this one to avoid rebasing the changes yet more as other dev continues
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261235218
Given it was only a 2 character change to a placeholder, which was clearly wrong before, I just squashed the fixup commit into the main one.
If you want to merge now I'm fine with that, would be nice not to rebase it again :)
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261019813
Just pushed a little fixup I spotted...will keep any changes as separate commits for now until its being merged, to make them easier to spot.
--
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 pull request #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
tabish121 commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261043627
Possible future enhancement would be to replace hard coding class names in logger definitions with Methodhandle lookups
`Logger logger = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());`
This tends to help when cutting and pasting loggers and forgetting to change the class name.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261283043
@gemmellr merging it...
I have one change to the log processor.. I will just do it after merged.
--
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 #4235: ARTEMIS-4020: use SLF4J logging API, with Log4J as impl for broker
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on PR #4235:
URL: https://github.com/apache/activemq-artemis/pull/4235#issuecomment-1261283346
YAY!!!!
Merged!!!!!
--
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