You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@activemq.apache.org by "Nemo Chen (JIRA)" <ji...@apache.org> on 2016/08/17 21:08:20 UTC
[jira] [Updated] (AMQ-6398) Several log refactoring/improvement
suggestion
[ 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.
*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:
*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.
> *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)