steveloughran commented on code in PR #5315:
URL: https://github.com/apache/hadoop/pull/5315#discussion_r1090640899
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/service/ServiceOperations.java:
##########
@@ -67,26 +66,6 @@ public static Exception stopQuietly(Service service) {
return stopQuietly(LOG, service);
}
- /**
- * Stop a service; if it is null do nothing. Exceptions are caught and
- * logged at warn level. (but not Throwables). This operation is intended to
- * be used in cleanup operations
- *
- * @param log the log to warn at
- * @param service a service; may be null
- * @return any exception that was caught; null if none was.
- * @see ServiceOperations#stopQuietly(Service)
- */
- public static Exception stopQuietly(Log log, Service service) {
Review Comment:
again, we should tag as deprecated in branch 3
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/ReflectionUtils.java:
##########
@@ -223,35 +222,6 @@ public synchronized static void
printThreadInfo(PrintStream stream,
private static long previousLogTime = 0;
- /**
- * Log the current thread stacks at INFO level.
- * @param log the logger that logs the stack trace
- * @param title a descriptive title for the call stacks
- * @param minInterval the minimum time from the last
- */
- public static void logThreadInfo(Log log,
Review Comment:
deprecate in 3.3
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/IOUtils.java:
##########
@@ -256,7 +255,7 @@ public static void skipFully(InputStream in, long len)
throws IOException {
* instead
*/
@Deprecated
- public static void cleanup(Log log, java.io.Closeable... closeables) {
+ public static void cleanup(Logger log, java.io.Closeable... closeables) {
Review Comment:
* for 3.3.5+ do a pr which tags as deprecated "will be cut" for early
warning.
* then do 1 or 2 here.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java:
##########
@@ -360,7 +358,8 @@ public class DataNode extends ReconfigurableBase
FS_GETSPACEUSED_JITTER_KEY,
FS_GETSPACEUSED_CLASSNAME));
- public static final Log METRICS_LOG =
LogFactory.getLog("DataNodeMetricsLog");
+ public static final org.apache.log4j.Logger METRICS_LOG =
+ org.apache.log4j.Logger.getLogger("DataNodeMetricsLog");
Review Comment:
is there a way the log4j refs can be kept inside MetricsLoggerTask...for
example, `makeMetricsLoggerAsync` could take a log name as a string and do the
lookup itself?
this would leave open a future option to move to reflection for the target
log platform
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java:
##########
@@ -949,13 +947,13 @@ protected void startMetricsLogger(Configuration conf) {
return;
}
- MetricsLoggerTask.makeMetricsLoggerAsync(MetricsLog);
+ MetricsLoggerTask.makeMetricsLoggerAsync(METRICS_LOG);
// Schedule the periodic logging.
metricsLoggerTimer = new ScheduledThreadPoolExecutor(1);
metricsLoggerTimer.setExecuteExistingDelayedTasksAfterShutdownPolicy(
false);
- metricsLoggerTimer.scheduleWithFixedDelay(new MetricsLoggerTask(MetricsLog,
+ metricsLoggerTimer.scheduleWithFixedDelay(new
MetricsLoggerTask(METRICS_LOG,
Review Comment:
and we would take a log name here
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java:
##########
@@ -949,13 +947,13 @@ protected void startMetricsLogger(Configuration conf) {
return;
}
- MetricsLoggerTask.makeMetricsLoggerAsync(MetricsLog);
+ MetricsLoggerTask.makeMetricsLoggerAsync(METRICS_LOG);
Review Comment:
again, a version of this method which took a log name would handle this
internally.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java:
##########
@@ -8853,16 +8849,12 @@ public void logAuditEvent(boolean succeeded, String
userName,
}
public void logAuditMessage(String message) {
- auditLog.info(message);
+ AUDIT_LOG.info(message);
}
}
private static void enableAsyncAuditLog(Configuration conf) {
- if (!(auditLog instanceof Log4JLogger)) {
Review Comment:
this is a key diff in the patch. today if the backend isn't log4j things
should work, just not do async logging (remember, logback doesn't need
this...), so this is possibly a regression
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/log/LogLevel.java:
##########
@@ -51,6 +47,7 @@
import org.apache.hadoop.util.ServletUtil;
import org.apache.hadoop.util.Tool;
import org.apache.hadoop.util.ToolRunner;
+import org.apache.log4j.Logger;
Review Comment:
with this as an explicit import, `process(org.apache.log4j.Logger log, ...)`
should remove its fully qualified import
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
