activemq-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nemo Chen (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (AMQ-6398) Several log refactoring/improvement suggestion
Date Wed, 17 Aug 2016 21:08:20 GMT

     [ https://issues.apache.org/jira/browse/AMQ-6398?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Nemo Chen updated AMQ-6398:
---------------------------
    Description: 
This can be separated to different issues for fixing. For now, i am grouping all the logging
improvement in one issue.
*Method Invocation Replaced By Variable*
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java
line 2168,
{code}
String schedulerDirPath = schedulerDir.getAbsolutePath();
{code}
line 2179,
{code}
LOG.warn("Job Scheduler Store limit is " + schedulerLimit / (1024 * 1024) +
         " mb, whilst the data directory: " + schedulerDir.getAbsolutePath() +
         " only has " + dirFreeSpace / (1024 * 1024) + " mb of usable space - resetting to
" +
        dirFreeSpace / (1024 * 1024) + " mb.");
{code}
"schedulerDir.getAbsolutePath()" can be replaced by "schedulerDirPath" for the sake of simplicity
and readability.

Similarly, in file:
activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java
line 1624:
{code}
QueueBrowserSubscription browser = browserDispatch.getBrowser();
{code}
line 1643:
{code}
LOG.warn("exception on dispatch to browser: {}", browserDispatch.getBrowser(), e);
{code}
the method invocation "browserDispatch.getBrowser()" can be replaced by "browser" for the
sake of simplicity and readability.

----
*Method Invocation in Return Statement*
Similar to fix for AMQ-4607,
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
line 350,
{code}
SERVICELOG.warn("Security Error occurred on connection to: {}, {}", transport.getRemoteAddress(),
e.getMessage());
{code}
line 1512,
{code}
@Override
public String getRemoteAddress() {
    return transport.getRemoteAddress();
}
{code}
"transport.getRemoteAddress()" can be replaced by method "getRemoteAddress" for the sake of
simplicity and readability

In file: activemq-parent-5.14.0/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java
line 756,
{code}
LOG.debug("{} clearing unconsumed list ({}) on transport interrupt", getConsumerId(), unconsumedMessages.size());
...
public int getMessageSize() {
    return unconsumedMessages.size();
}

{code}
"unconsumedMessages.size()" can be replaced by method "getMessageSize" for the sake of simplicity
and readability

----
*Printing null in logs*
In file: /activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
{code}
@Override
public Response processMessageAck(MessageAck ack) throws Exception {
    ConsumerBrokerExchange consumerExchange = getConsumerBrokerExchange(ack.getConsumerId());
    if (consumerExchange != null) {
        broker.acknowledge(consumerExchange, ack);
    } else if (ack.isInTransaction()) {
        LOG.warn("no matching consumer, ignoring ack {}", consumerExchange, ack);
    }
    return null;
}
{code}
in the log, the consumerExchange must be null. Also it is weird to print two variables with
only one "{}"

----
*Variable is byte*
In file: activemq-parent-5.14.0/activemq-kahadb-store/src/test/java/org/apache/activemq/store/kahadb/JournalCorruptionEofIndexRecoveryTest.java
{code}
int size = randomAccessFile.readInt();
byte type = randomAccessFile.readByte();

LOG.info("Read: size:" + size + ", type:" + type);
{code}
Should type be replaced with "Bytes.toString(type)"?

----
*Variable toString contain the Method invocation*
Similar to the fix for AMQ-3159,
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerRegistry.java
{code}
LOG.warn("Broker localhost not started so using {} instead", result.getBrokerName());
{code}
The variable "result" is an instance of class "BrokerService", the toString method of this
class is:
{code}
public String toString() {
    return "BrokerService[" + getBrokerName() + "]";
}
{code}
Therefore we can replace "result.getBrokerName()" with "result"

In file: activemq-parent-5.14.0/activemq-leveldb-store/src/test/java/org/apache/activemq/leveldb/test/ReplicatedLevelDBBrokerTest.java
Similar problem as the previous one:
{code}
System.out.println("Stopping slave: " + broker.getBrokerName());
{code}
"broker.getBrokerName()" can be replaced with "broker"
{code}
System.out.println("Master started: " + master.getBrokerName());
{code}
"master.getBrokerName" can be replaced with "master"

  was:
This can be separated to different issues for fixing.
*Method Invocation Replaced By Variable*
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java
line 2168,
{code}
String schedulerDirPath = schedulerDir.getAbsolutePath();
{code}
line 2179,
{code}
LOG.warn("Job Scheduler Store limit is " + schedulerLimit / (1024 * 1024) +
         " mb, whilst the data directory: " + schedulerDir.getAbsolutePath() +
         " only has " + dirFreeSpace / (1024 * 1024) + " mb of usable space - resetting to
" +
        dirFreeSpace / (1024 * 1024) + " mb.");
{code}
"schedulerDir.getAbsolutePath()" can be replaced by "schedulerDirPath" for the sake of simplicity
and readability.

Similarly, in file:
activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java
line 1624:
{code}
QueueBrowserSubscription browser = browserDispatch.getBrowser();
{code}
line 1643:
{code}
LOG.warn("exception on dispatch to browser: {}", browserDispatch.getBrowser(), e);
{code}
the method invocation "browserDispatch.getBrowser()" can be replaced by "browser" for the
sake of simplicity and readability.

----
*Method Invocation in Return Statement*
Similar to fix for AMQ-4607,
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
line 350,
{code}
SERVICELOG.warn("Security Error occurred on connection to: {}, {}", transport.getRemoteAddress(),
e.getMessage());
{code}
line 1512,
{code}
@Override
public String getRemoteAddress() {
    return transport.getRemoteAddress();
}
{code}
"transport.getRemoteAddress()" can be replaced by method "getRemoteAddress" for the sake of
simplicity and readability

In file: activemq-parent-5.14.0/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java
line 756,
{code}
LOG.debug("{} clearing unconsumed list ({}) on transport interrupt", getConsumerId(), unconsumedMessages.size());
...
public int getMessageSize() {
    return unconsumedMessages.size();
}

{code}
"unconsumedMessages.size()" can be replaced by method "getMessageSize" for the sake of simplicity
and readability

----
*Printing null in logs*
In file: /activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
{code}
@Override
public Response processMessageAck(MessageAck ack) throws Exception {
    ConsumerBrokerExchange consumerExchange = getConsumerBrokerExchange(ack.getConsumerId());
    if (consumerExchange != null) {
        broker.acknowledge(consumerExchange, ack);
    } else if (ack.isInTransaction()) {
        LOG.warn("no matching consumer, ignoring ack {}", consumerExchange, ack);
    }
    return null;
}
{code}
in the log, the consumerExchange must be null. Also it is weird to print two variables with
only one "{}"

----
*Variable is byte*
In file: activemq-parent-5.14.0/activemq-kahadb-store/src/test/java/org/apache/activemq/store/kahadb/JournalCorruptionEofIndexRecoveryTest.java
{code}
int size = randomAccessFile.readInt();
byte type = randomAccessFile.readByte();

LOG.info("Read: size:" + size + ", type:" + type);
{code}
Should type be replaced with "Bytes.toString(type)"?

----
*Variable toString contain the Method invocation*
Similar to the fix for AMQ-3159,
In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerRegistry.java
{code}
LOG.warn("Broker localhost not started so using {} instead", result.getBrokerName());
{code}
The variable "result" is an instance of class "BrokerService", the toString method of this
class is:
{code}
public String toString() {
    return "BrokerService[" + getBrokerName() + "]";
}
{code}
Therefore we can replace "result.getBrokerName()" with "result"

In file: activemq-parent-5.14.0/activemq-leveldb-store/src/test/java/org/apache/activemq/leveldb/test/ReplicatedLevelDBBrokerTest.java
Similar problem as the previous one:
{code}
System.out.println("Stopping slave: " + broker.getBrokerName());
{code}
"broker.getBrokerName()" can be replaced with "broker"
{code}
System.out.println("Master started: " + master.getBrokerName());
{code}
"master.getBrokerName" can be replaced with "master"


> Several log refactoring/improvement suggestion
> ----------------------------------------------
>
>                 Key: AMQ-6398
>                 URL: https://issues.apache.org/jira/browse/AMQ-6398
>             Project: ActiveMQ
>          Issue Type: Bug
>    Affects Versions: 5.14.0
>            Reporter: Nemo Chen
>              Labels: easyfix, easytest
>
> This can be separated to different issues for fixing. For now, i am grouping all the
logging improvement in one issue.
> *Method Invocation Replaced By Variable*
> In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerService.java
> line 2168,
> {code}
> String schedulerDirPath = schedulerDir.getAbsolutePath();
> {code}
> line 2179,
> {code}
> LOG.warn("Job Scheduler Store limit is " + schedulerLimit / (1024 * 1024) +
>          " mb, whilst the data directory: " + schedulerDir.getAbsolutePath() +
>          " only has " + dirFreeSpace / (1024 * 1024) + " mb of usable space - resetting
to " +
>         dirFreeSpace / (1024 * 1024) + " mb.");
> {code}
> "schedulerDir.getAbsolutePath()" can be replaced by "schedulerDirPath" for the sake of
simplicity and readability.
> Similarly, in file:
> activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java
> line 1624:
> {code}
> QueueBrowserSubscription browser = browserDispatch.getBrowser();
> {code}
> line 1643:
> {code}
> LOG.warn("exception on dispatch to browser: {}", browserDispatch.getBrowser(), e);
> {code}
> the method invocation "browserDispatch.getBrowser()" can be replaced by "browser" for
the sake of simplicity and readability.
> ----
> *Method Invocation in Return Statement*
> Similar to fix for AMQ-4607,
> In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
> line 350,
> {code}
> SERVICELOG.warn("Security Error occurred on connection to: {}, {}", transport.getRemoteAddress(),
e.getMessage());
> {code}
> line 1512,
> {code}
> @Override
> public String getRemoteAddress() {
>     return transport.getRemoteAddress();
> }
> {code}
> "transport.getRemoteAddress()" can be replaced by method "getRemoteAddress" for the sake
of simplicity and readability
> In file: activemq-parent-5.14.0/activemq-client/src/main/java/org/apache/activemq/ActiveMQMessageConsumer.java
> line 756,
> {code}
> LOG.debug("{} clearing unconsumed list ({}) on transport interrupt", getConsumerId(),
unconsumedMessages.size());
> ...
> public int getMessageSize() {
>     return unconsumedMessages.size();
> }
> {code}
> "unconsumedMessages.size()" can be replaced by method "getMessageSize" for the sake of
simplicity and readability
> ----
> *Printing null in logs*
> In file: /activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/TransportConnection.java
> {code}
> @Override
> public Response processMessageAck(MessageAck ack) throws Exception {
>     ConsumerBrokerExchange consumerExchange = getConsumerBrokerExchange(ack.getConsumerId());
>     if (consumerExchange != null) {
>         broker.acknowledge(consumerExchange, ack);
>     } else if (ack.isInTransaction()) {
>         LOG.warn("no matching consumer, ignoring ack {}", consumerExchange, ack);
>     }
>     return null;
> }
> {code}
> in the log, the consumerExchange must be null. Also it is weird to print two variables
with only one "{}"
> ----
> *Variable is byte*
> In file: activemq-parent-5.14.0/activemq-kahadb-store/src/test/java/org/apache/activemq/store/kahadb/JournalCorruptionEofIndexRecoveryTest.java
> {code}
> int size = randomAccessFile.readInt();
> byte type = randomAccessFile.readByte();
> LOG.info("Read: size:" + size + ", type:" + type);
> {code}
> Should type be replaced with "Bytes.toString(type)"?
> ----
> *Variable toString contain the Method invocation*
> Similar to the fix for AMQ-3159,
> In file: activemq-parent-5.14.0/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerRegistry.java
> {code}
> LOG.warn("Broker localhost not started so using {} instead", result.getBrokerName());
> {code}
> The variable "result" is an instance of class "BrokerService", the toString method of
this class is:
> {code}
> public String toString() {
>     return "BrokerService[" + getBrokerName() + "]";
> }
> {code}
> Therefore we can replace "result.getBrokerName()" with "result"
> In file: activemq-parent-5.14.0/activemq-leveldb-store/src/test/java/org/apache/activemq/leveldb/test/ReplicatedLevelDBBrokerTest.java
> Similar problem as the previous one:
> {code}
> System.out.println("Stopping slave: " + broker.getBrokerName());
> {code}
> "broker.getBrokerName()" can be replaced with "broker"
> {code}
> System.out.println("Master started: " + master.getBrokerName());
> {code}
> "master.getBrokerName" can be replaced with "master"



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message