Skip to content

HADOOP-17482: Remove Commons Logger from FileSystem Class #2633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
goiri merged 2 commits into from
Feb 1, 2021
Merged

HADOOP-17482: Remove Commons Logger from FileSystem Class #2633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b6d0e9
HADOOP-17482: Remove Commons Logger from FileSystem Class
belugabehr Jan 20, 2021
188a33c
Fix checkstyle, warnings, etc.
belugabehr Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions ...mmon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/DelegationTokenRenewer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ public <T extends FileSystem & Renewable> RenewAction<T> addRenewAction(final T
if (action.token != null) {
queue.add(action);
} else {
fs.LOG.error("does not have a token for renewal");
FileSystem.LOG.error("does not have a token for renewal");
}
return action;
}
Expand All @@ -247,7 +247,6 @@ public <T extends FileSystem & Renewable> void removeRenewAction(
}
}

@SuppressWarnings("static-access")
@Override
public void run() {
for(;;) {
Expand All @@ -260,8 +259,7 @@ public void run() {
} catch (InterruptedException ie) {
return;
} catch (Exception ie) {
action.weakFs.get().LOG.warn("Failed to renew token, action=" + action,
ie);
FileSystem.LOG.warn("Failed to renew token, action=" + action, ie);
}
}
}
Expand Down
14 changes: 2 additions & 12 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@
import java.util.concurrent.Semaphore;
import java.util.concurrent.atomic.AtomicLong;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.classification.InterfaceStability;
import org.apache.hadoop.conf.Configuration;
Expand Down Expand Up @@ -183,7 +181,7 @@ public abstract class FileSystem extends Configured
* so must be considered something to only be changed with care.
*/
@InterfaceAudience.Private
public static final Log LOG = LogFactory.getLog(FileSystem.class);
public static final Logger LOG = LoggerFactory.getLogger(FileSystem.class);

/**
* The SLF4J logger to use in logging within the FileSystem class itself.
Expand Down Expand Up @@ -3391,15 +3389,7 @@ private static void loadFileSystems() {
LOGGER.info("Full exception loading: {}", fs, e);
}
} catch (ServiceConfigurationError ee) {
LOG.warn("Cannot load filesystem: " + ee);
Throwable cause = ee.getCause();
// print all the nested exception messages
while (cause != null) {
LOG.warn(cause.toString());
cause = cause.getCause();
}
// and at debug: the full stack
LOG.debug("Stack Trace", ee);
LOGGER.warn("Cannot load filesystem", ee);
Copy link
Copy Markdown
Contributor Author

@belugabehr belugabehr Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the capability of Commons Logger is, but in SLF4J, the entire list of 'caused by' is printed so there is no need to do this manually when switching loggers, but this may also just be an oversight, I'm not sure.

Copy link
Copy Markdown
Member

@goiri goiri Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think relying on slf4j and just dumping the whole exception is good.

}
}
FILE_SYSTEMS_LOADED = true;
Expand Down
2 changes: 1 addition & 1 deletion ...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/RawLocalFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ private LocalFSFileOutputStream(Path f, boolean append,
success = true;
} finally {
if (!success) {
IOUtils.cleanup(LOG, this.fos);
IOUtils.cleanupWithLogger(LOG, this.fos);
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,10 +630,9 @@ protected InodeTree(final Configuration config, final String viewName,
.append(theUri.getScheme()).append("://").append(mountTableName)
.append("/").toString());
}
StringBuilder msg =
new StringBuilder("Empty mount table detected for ").append(theUri)
.append(" and considering itself as a linkFallback.");
FileSystem.LOG.info(msg.toString());
FileSystem.LOG
.info("Empty mount table detected for {} and considering itself "
+ "as a linkFallback.", theUri);
rootFallbackLink =
new INodeLink<T>(mountTableName, ugi, getTargetFileSystem(theUri),
theUri);
Expand Down
13 changes: 4 additions & 9 deletions ...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,10 +1289,8 @@ public FSDataOutputStream create(final Path f,
.create(fileToCreate, permission, overwrite, bufferSize,
replication, blockSize, progress);
} catch (IOException e) {
StringBuilder msg =
new StringBuilder("Failed to create file:").append(fileToCreate)
.append(" at fallback : ").append(linkedFallbackFs.getUri());
LOG.error(msg.toString(), e);
LOG.error("Failed to create file: {} at fallback: {}", fileToCreate,
linkedFallbackFs.getUri(), e);
throw e;
}
}
Expand Down Expand Up @@ -1523,11 +1521,8 @@ public boolean mkdirs(Path dir, FsPermission permission)
return linkedFallbackFs.mkdirs(dirToCreate, permission);
} catch (IOException e) {
if (LOG.isDebugEnabled()) {
StringBuilder msg =
new StringBuilder("Failed to create ").append(dirToCreate)
.append(" at fallback : ")
.append(linkedFallbackFs.getUri());
LOG.debug(msg.toString(), e);
LOG.debug("Failed to create: {} at fallback: {}", dirToCreate,
linkedFallbackFs.getUri(), e);
}
throw e;
}
Expand Down
4 changes: 2 additions & 2 deletions ...common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFilterFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.util.EnumSet;
import java.util.Iterator;

import org.apache.commons.logging.Log;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.fs.Options.CreateOpts;
Expand All @@ -39,10 +38,11 @@
import org.apache.hadoop.util.Progressable;
import org.junit.BeforeClass;
import org.junit.Test;
import org.slf4j.Logger;

public class TestFilterFileSystem {

private static final Log LOG = FileSystem.LOG;
private static final Logger LOG = FileSystem.LOG;
private static final Configuration conf = new Configuration();

@BeforeClass
Expand Down
4 changes: 2 additions & 2 deletions hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFilterFs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@
import java.net.URI;
import java.util.Iterator;

import org.apache.commons.logging.Log;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.viewfs.ConfigUtil;
import org.junit.Test;
import org.slf4j.Logger;

public class TestFilterFs {

private static final Log LOG = FileSystem.LOG;
private static final Logger LOG = FileSystem.LOG;

public static class DontCheck {
public void checkScheme(URI uri, String supportedScheme) { }
Expand Down
13 changes: 0 additions & 13 deletions hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestFcHdfsSetUMask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@
import org.apache.hadoop.hdfs.HdfsConfiguration;
import org.apache.hadoop.hdfs.MiniDFSCluster;
import org.apache.hadoop.security.UserGroupInformation;
import org.apache.hadoop.test.GenericTestUtils;
import org.apache.hadoop.util.StringUtils;
import static org.apache.hadoop.fs.FileContextTestHelper.*;
import org.apache.log4j.Level;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Assert;
Expand Down Expand Up @@ -100,16 +97,6 @@ public static void ClusterShutdownAtEnd() throws Exception {
cluster.shutdown();
}
}

{
try {
GenericTestUtils.setLogLevel(FileSystem.LOG, Level.DEBUG);
}
catch(Exception e) {
System.out.println("Cannot change log level\n"
+ StringUtils.stringifyException(e));
}
}
Comment on lines -103 to -112
Copy link
Copy Markdown
Contributor Author

@belugabehr belugabehr Jan 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not good practice to modify this manually. Should be using log4j properties files to set logging levels.

Copy link
Copy Markdown
Contributor

@steveloughran steveloughran Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a test and they may have had some good reasons, just as some of the Azure tests do

Copy link
Copy Markdown
Member

@goiri goiri Feb 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to fix it properly then. @belugabehr do you mind undoing this particular one in a separate JIRA or proposing the log4j solution?


@Before
public void setUp() throws Exception {
Expand Down
7 changes: 0 additions & 7 deletions hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestListFilesInDFS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,15 @@
*/
package org.apache.hadoop.hdfs;

import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.TestListFiles;
import org.apache.hadoop.test.GenericTestUtils;
import org.apache.log4j.Level;
import org.junit.AfterClass;
import org.junit.BeforeClass;

/**
* This class tests the FileStatus API.
*/
public class TestListFilesInDFS extends TestListFiles {
{
GenericTestUtils.setLogLevel(FileSystem.LOG, Level.ALL);
}


private static MiniDFSCluster cluster;

Expand Down
6 changes: 0 additions & 6 deletions ...-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestListFilesInFileContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,11 @@
import org.apache.hadoop.fs.CreateFlag;
import org.apache.hadoop.fs.FSDataOutputStream;
import org.apache.hadoop.fs.FileContext;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.LocatedFileStatus;
import org.apache.hadoop.fs.Options;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.RemoteIterator;
import org.apache.hadoop.fs.permission.FsPermission;
import org.apache.hadoop.test.GenericTestUtils;
import org.apache.log4j.Level;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.BeforeClass;
Expand All @@ -46,9 +43,6 @@
* This class tests the FileStatus API.
*/
public class TestListFilesInFileContext {
{
GenericTestUtils.setLogLevel(FileSystem.LOG, Level.ALL);
}

static final long seed = 0xDEADBEEFL;

Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/org/apache/hadoop/yarn/server/nodemanager/WindowsSecureContainerExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ protected OutputStream createOutputStreamWithMode(Path f, boolean append,
return os;
} finally {
if (!success) {
IOUtils.cleanup(LOG, os);
IOUtils.cleanupWithLogger(LOG, os);
}
}
}
Expand Down