xinglin commented on code in PR #5817:
URL: https://github.com/apache/hadoop/pull/5817#discussion_r1256116824
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockChecksumHelper.java:
##########
@@ -493,7 +493,8 @@ void compute() throws IOException {
checksumBlock(block, idx, liveBlkInfo.getToken(),
liveBlkInfo.getDn());
} catch (IOException ioe) {
- LOG.warn("Exception while reading checksum", ioe);
+ LOG.warn("Exception while reading checksum for block {} at index
{} " +
+ "in blockGroup {}", block, idx, blockGroup, ioe);
Review Comment:
Hi,
We only have a definition as `warn(final String format, final Throwable t)
`, which accepts a throwable object for LOG.warn(). For the others, the
exception is just treated as a regular object.
```
public void warn(final String format) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format);
}
public void warn(final String format, final Object o) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, o);
}
public void warn(final String format, final Object arg1, final Object
arg2) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, arg1, arg2);
}
public void warn(final String format, final Object... args) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, args);
}
public void warn(final String format, final Throwable t) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, t);
}
```
Maybe we should do something like the following:
```
String msg = String.format("Exception while reading checksum for block
%s at index %d " +
"in blockGroup %s", block, idx, blockGroup);
LOG.warn(msg, e);
```
Other than this, the change looks good.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockChecksumHelper.java:
##########
@@ -493,7 +493,8 @@ void compute() throws IOException {
checksumBlock(block, idx, liveBlkInfo.getToken(),
liveBlkInfo.getDn());
} catch (IOException ioe) {
- LOG.warn("Exception while reading checksum", ioe);
+ LOG.warn("Exception while reading checksum for block {} at index
{} " +
+ "in blockGroup {}", block, idx, blockGroup, ioe);
Review Comment:
Hi,
We only have a definition as `warn(final String format, final Throwable t)
`, which accepts a throwable object for LOG.warn(). For the others, the
exception is just treated as a regular object.
```
public void warn(final String format) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format);
}
public void warn(final String format, final Object o) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, o);
}
public void warn(final String format, final Object arg1, final Object
arg2) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, arg1, arg2);
}
public void warn(final String format, final Object... args) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, args);
}
public void warn(final String format, final Throwable t) {
this.logger.logIfEnabled(FQCN, Level.WARN,
(org.apache.logging.log4j.Marker)null, format, t);
}
```
Maybe we should do something like the following:
```
String msg = String.format("Exception while reading checksum for block
%s at index %d " +
"in blockGroup %s", block, idx, blockGroup);
LOG.warn(msg, e);
```
Other than that, the change looks good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
