Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 8AD01200B65 for ; Wed, 17 Aug 2016 23:09:22 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 8954E160AB5; Wed, 17 Aug 2016 21:09:22 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id A533E160A8C for ; Wed, 17 Aug 2016 23:09:21 +0200 (CEST) Received: (qmail 12698 invoked by uid 500); 17 Aug 2016 21:09:20 -0000 Mailing-List: contact issues-help@activemq.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@activemq.apache.org Delivered-To: mailing list issues@activemq.apache.org Received: (qmail 12530 invoked by uid 99); 17 Aug 2016 21:09:20 -0000 Received: from arcas.apache.org (HELO arcas) (140.211.11.28) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 17 Aug 2016 21:09:20 +0000 Received: from arcas.apache.org (localhost [127.0.0.1]) by arcas (Postfix) with ESMTP id A3B512C02A3 for ; Wed, 17 Aug 2016 21:09:20 +0000 (UTC) Date: Wed, 17 Aug 2016 21:09:20 +0000 (UTC) From: "Nemo Chen (JIRA)" To: issues@activemq.apache.org Message-ID: In-Reply-To: References: Subject: [jira] [Updated] (AMQ-6398) Several log refactoring/improvement suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394 archived-at: Wed, 17 Aug 2016 21:09:22 -0000 [ https://issues.apache.org/jira/browse/AMQ-6398?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Nemo Chen updated AMQ-6398: --------------------------- Description: For now, i am grouping all the logging improvement in one issue.But let me know otherwise, I can file separate jira issues. *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. 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" > 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 > > For now, i am grouping all the logging improvement in one issue.But let me know otherwise, I can file separate jira issues. > *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)