[jira] [Commented] (AMQ-8601) UpdateVirtualDestinationsTask gives inaccurate log message saying "Removing virtual destination ... " after already applied the removal

["Ding Ding (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/AMQ-8601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17580354#comment-17580354 ] 

Ding Ding commented on AMQ-8601:
--------------------------------

Thanks for checking this report.  We made a PR for this issue.  [https://github.com/apache/activemq/pull/840]

 

Besides, would you mind checking the following cases, there are also some cases that may be improved: (if needed, we can also create another PR. :))

1) *Logging statement:*  [https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-kahadb-store/src/main/java/org/apache/activemq/store/kahadb/scheduler/legacy/LegacyStoreReplayer.java/#L117]

 
{code:java}
LOG.info("Replay of legacy store complate."); {code}
 

*Suggestion:* Change "complate"{{ }}to "has completed."
*Reason:* 1)Typo;  2)The logging statement is placed at the end of the block indicting the function has been successfully executed.

 

2) *Logging statement:*  [https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java/#L1123]
{code:java}
LOG.info("Stopping {} because {}", transport.getRemoteAddress(), reason);{code}
*Suggestion:* Change "stopping"{{ }}to "Stopped."
*Reason:* The logging statement is placed after the function stopAsync() indicting the function has been successfully executed.

 

3) *Logging statement:*  [https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java/#L2159]
{code:java}
LOG.warn("{} mb - resetting to 70% of maximum available: {}",                        
         message, (usage.getMemoryUsage().getLimit() / (1024 * 1024)));{code}
*Suggestion:* Change "resetting"{{ }}to "reset."
*Reason:* The logging statement is placed after the function s.setPercentOfJvmHeap(70) indicting the function has been successfully executed.

 

4) *Logging statement:*  [https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-broker/src/main/java/org/apache/activemq/broker/region/cursors/AbstractStoreCursor.java/#L280]
{code:java}
LOG.trace("{} enabling cache on empty store", this);{code}
*Suggestion:* Change "enabling"{{ }}to "has enabled."
*Reason:* The logging statement is placed after the function setCacheEnabled(true)  indicting the function has been successfully executed.

 

5) *Logging statement:*  [https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-broker/src/main/java/org/apache/activemq/network/DemandForwardingBridgeSupport.java/#L689]
{code:java}
LOG.debug("Adding proxy network subscription {} to demand subscription", newSubInfo);{code}
*Suggestion:* Change "Adding"{{ }}to "Added."
*Reason:* The logging statement is placed after the function add(newSubInfo) indicting the function has been successfully executed.

Thank you.

> UpdateVirtualDestinationsTask gives inaccurate log message saying "Removing virtual destination ... " after already applied the removal
> ---------------------------------------------------------------------------------------------------------------------------------------
>
>                 Key: AMQ-8601
>                 URL: https://issues.apache.org/jira/browse/AMQ-8601
>             Project: ActiveMQ
>          Issue Type: Bug
>            Reporter: Ding Ding
>            Assignee: Jean-Baptiste Onofré
>            Priority: Trivial
>             Fix For: 5.18.0, 5.17.2
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> Hello,
> While viewing the MAPREDUCE-4262, I found that the logging statements might give inaccurate messages. 
> I also found that the in the line *93* of the file [UpdateVirtualDestinationsTask|https://github.com/apache/activemq/blob/b3c2c49f96bebee14b74c6456feb233e0312724b/activemq-runtime-config/src/main/java/org/apache/activemq/plugin/UpdateVirtualDestinationsTask.java#L93], the log messages says "{_}Removing virtual destination  "{_}. However, the removing action should already completed in previous code ({*}line 92{*}).
> {code:java}
> plugin.virtualDestinationRemoved(connectionContext, removedVirtualDest);
> LOG.info("Removing virtual destination: {}", removedVirtualDest); {code}
> Would it be better if we change the verb "Removing" to "Removed" to indicate the action is completed, which is similar to the previous logging statement?
> {code:java}
> virtualDestinationInterceptor.setVirtualDestinations(getVirtualDestinations());                
> plugin.info("applied updates to: " + virtualDestinationInterceptor); {code}
> Or can we move the logging statement to the line before {*}92{*}? Since when there was an exception in previous lines, the logging message would not be printed, which may be not good for debugging. 
>  
> A similar issue is also found in the file [StatisticsBrokerPlugin|https://github.com/apache/activemq/blob/9b1eb96b838957cd60541ca5e057567be3f11990/activemq-broker/src/main/java/org/apache/activemq/plugin/StatisticsBrokerPlugin.java#L47] where it seems that the logging statement it is describing the whole method. Would it be better to move this logging statement to the beginning of the method? 
> Especially considering the following logging practices:
>  # [https://github.com/apache/activemq/blob/9b1eb96b838957cd60541ca5e057567be3f11990/activemq-broker/src/main/java/org/apache/activemq/plugin/DiscardingDLQBrokerPlugin.java#L56]
>  # [https://github.com/apache/activemq/blob/9b1eb96b838957cd60541ca5e057567be3f11990/activemq-runtime-config/src/main/java/org/apache/activemq/plugin/RuntimeConfigurationPlugin.java#L37] 
> Thanks.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)