[ https://issues.apache.org/jira/browse/HBASE-16469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Nemo Chen updated HBASE-16469: ------------------------------ Description: *method invocation replaced by variable* hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java line 118: {code}String name = regionInfo.getRegionNameAsString();{code} line 142: {code}LOG.warn("Can't close region: was already closed during close(): " + regionInfo.getRegionNameAsString()); {code} In the above two examples, the method invocations are assigned to the variables before the logging code. These method invocations should be replaced by variables in case of simplicity and readability ---- *method invocation in return statement* hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java line 5455: {code} public String toString() { return getRegionInfo().getRegionNameAsString(); } {code} line 1260: {code} LOG.debug("Region " + getRegionInfo().getRegionNameAsString() + " is not mergeable because it is closing or closed"); {code} line 1265: {code} LOG.debug("Region " + getRegionInfo().getRegionNameAsString() + " is not mergeable because it has references"); {code} line 1413: {code} LOG.info("Running close preflush of " + getRegionInfo().getRegionNameAsString()); {code} In these above examples, the "getRegionInfo().getRegionNameAsString())" is the return statement of method "toString" in the same class. They should be replaced with “this” in case of simplicity and readability. ---- *check the logged variable if it is null* hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java line 88: {code} if ((sshUserName != null && sshUserName.length() > 0) || (sshOptions != null && sshOptions.length() > 0)) { LOG.info("Running with SSH user [" + sshUserName + "] and options [" + sshOptions + "]"); } {code} hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java line 980: {code} if ((regionState == null && latestState != null) || (regionState != null && latestState == null) || (regionState != null && latestState != null && latestState.getState() != regionState.getState())) { LOG.warn("Region state changed from " + regionState + " to " + latestState + ", while acquiring lock"); } {code} In the above example, the logging variable could null at run time. It is a bad practice to include null variables inside logs. ---- *variable in byte printed directly* hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedUpdater.java line 145: {code} byte[] rowKey = dataGenerator.getDeterministicUniqueKey(rowKeyBase); {code} line 184: {code} LOG.error("Failed to update the row with key = [" + rowKey + "], since we could not get the original row"); {code} rowKey should be printed as Bytes.toString(rowKey). ---- *object toString contain mi* The toString method returns getServerName(), so the "server.getServerName()" should be replaced with "server" in case of simplicity and readability. hbase-client/src/main/java/org/apache/hadoop/hbase/client/PreemptiveFastFailInterceptor.java {code} LOG.info("Clearing out PFFE for server " + server.getServerName()); return getServerName(); {code} was: *method invocation replaced by variable* hbase-1.2.2/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java line 118: {code}String name = regionInfo.getRegionNameAsString();{code} line 142: {code}LOG.warn("Can't close region: was already closed during close(): " + regionInfo.getRegionNameAsString()); {code} In the above two examples, the method invocations are assigned to the variables before the logging code. These method invocations should be replaced by variables in case of simplicity and readability ---- *method invocation in return statement* hbase-1.2.2/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java line 5455: {code} public String toString() { return getRegionInfo().getRegionNameAsString(); } {code} line 1260: {code} LOG.debug("Region " + getRegionInfo().getRegionNameAsString() + " is not mergeable because it is closing or closed"); {code} line 1265: {code} LOG.debug("Region " + getRegionInfo().getRegionNameAsString() + " is not mergeable because it has references"); {code} line 1413: {code} LOG.info("Running close preflush of " + getRegionInfo().getRegionNameAsString()); {code} In these above examples, the "getRegionInfo().getRegionNameAsString())" is the return statement of method "toString" in the same class. They should be replaced with “this” in case of simplicity and readability. ---- *check the logged variable if it is null* hbase-1.2.2/hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java line 88: {code} if ((sshUserName != null && sshUserName.length() > 0) || (sshOptions != null && sshOptions.length() > 0)) { LOG.info("Running with SSH user [" + sshUserName + "] and options [" + sshOptions + "]"); } {code} hbase-1.2.2/hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java line 980: {code} if ((regionState == null && latestState != null) || (regionState != null && latestState == null) || (regionState != null && latestState != null && latestState.getState() != regionState.getState())) { LOG.warn("Region state changed from " + regionState + " to " + latestState + ", while acquiring lock"); } {code} In the above example, the logging variable could null at run time. It is a bad practice to include null variables inside logs. ---- *variable in byte printed directly* hbase-1.2.2/hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedUpdater.java line 145: {code} byte[] rowKey = dataGenerator.getDeterministicUniqueKey(rowKeyBase); {code} line 184: {code} LOG.error("Failed to update the row with key = [" + rowKey + "], since we could not get the original row"); {code} rowKey should be printed as Bytes.toString(rowKey). ---- *object toString contain mi* hbase-1.2.2/hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java {code} LOG.warn("#" + id + ", the task was rejected by the pool. This is unexpected."+ " Server is "+ server.getServerName(),t); {code} server is an instance of class ServerName, we found ServerName.java: hbase-client/src/main/java/org/apache/hadoop/hbase/ServerName.java {code} @Override public String toString() { return getServerName(); } {code} the toString method returns getServerName(), so the "server.getServerName()" should be replaced with "server" in case of simplicity and readability Similar examples are in: hbase-1.2.2/hbase-client/src/main/java/org/apache/hadoop/hbase/client/PreemptiveFastFailInterceptor.java {code} LOG.info("Clearing out PFFE for server " + server.getServerName()); return getServerName(); {code} hbase-1.2.2/hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/RpcServer.java line 705: {code} LOG.debug(getName() + ": disconnecting client " + c.getHostAddress()); {code} line 1259: {code} public String toString() { return getHostAddress() + ":" + remotePort; } {code} > Several log refactoring/improvement suggestions > ----------------------------------------------- > > Key: HBASE-16469 > URL: https://issues.apache.org/jira/browse/HBASE-16469 > Project: HBase > Issue Type: Improvement > Components: Operability > Affects Versions: 1.2.5 > Reporter: Nemo Chen > Assignee: Nemo Chen > Labels: easyfix, easytest > Fix For: 2.0.0, 1.5.0 > > Attachments: HBASE-16469.master.001.patch > > > *method invocation replaced by variable* > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/handler/CloseRegionHandler.java > line 118: {code}String name = regionInfo.getRegionNameAsString();{code} > line 142: {code}LOG.warn("Can't close region: was already closed during > close(): " + > regionInfo.getRegionNameAsString()); {code} > In the above two examples, the method invocations are assigned to the > variables before the logging code. These method invocations should be > replaced by variables in case of simplicity and readability > ---- > *method invocation in return statement* > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java > line 5455: > {code} > public String toString() { > return getRegionInfo().getRegionNameAsString(); > } > {code} > line 1260: > {code} > LOG.debug("Region " + getRegionInfo().getRegionNameAsString() > + " is not mergeable because it is closing or closed"); > {code} > line 1265: > {code} > LOG.debug("Region " + getRegionInfo().getRegionNameAsString() > + " is not mergeable because it has references"); > {code} > line 1413: > {code} > LOG.info("Running close preflush of " + > getRegionInfo().getRegionNameAsString()); > {code} > In these above examples, the "getRegionInfo().getRegionNameAsString())" is > the return statement of method "toString" in the same class. They should be > replaced with “this” in case of simplicity and readability. > ---- > *check the logged variable if it is null* > hbase-it/src/test/java/org/apache/hadoop/hbase/HBaseClusterManager.java > line 88: > {code} > if ((sshUserName != null && sshUserName.length() > 0) || > (sshOptions != null && sshOptions.length() > 0)) { > LOG.info("Running with SSH user [" + sshUserName + "] and options [" + > sshOptions + "]"); > } > {code} > hbase-server/src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java > line 980: > {code} > if ((regionState == null && latestState != null) > || (regionState != null && latestState == null) > || (regionState != null && latestState != null > && latestState.getState() != regionState.getState())) { > LOG.warn("Region state changed from " + regionState + " to " > + latestState + ", while acquiring lock"); > } > {code} > In the above example, the logging variable could null at run time. It is a > bad practice to include null variables inside logs. > ---- > *variable in byte printed directly* > hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedUpdater.java > line 145: > {code} > byte[] rowKey = dataGenerator.getDeterministicUniqueKey(rowKeyBase); > {code} > line 184: > {code} > LOG.error("Failed to update the row with key = [" + rowKey > + "], since we could not get the original row"); > {code} > rowKey should be printed as Bytes.toString(rowKey). > ---- > *object toString contain mi* > The toString method returns getServerName(), so the "server.getServerName()" > should be replaced with "server" in case of simplicity and readability. > hbase-client/src/main/java/org/apache/hadoop/hbase/client/PreemptiveFastFailInterceptor.java > {code} > LOG.info("Clearing out PFFE for server " + server.getServerName()); > return getServerName(); > {code} -- This message was sent by Atlassian JIRA (v6.3.15#6346)