From f5efc187e5490b2172e0f1d68ce5b528977c81ee Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Wed, 13 Apr 2011 00:09:12 +0000 Subject: [PATCH] HADOOP-7223. FileContext createFlag combinations are not clearly defined. Contributed by Suresh Srinivas. git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1091613 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES.txt | 3 + src/java/org/apache/hadoop/fs/ChecksumFs.java | 5 +- src/java/org/apache/hadoop/fs/CreateFlag.java | 82 +++++++++--- .../org/apache/hadoop/fs/FileContext.java | 7 +- src/java/org/apache/hadoop/fs/FileSystem.java | 17 ++- .../apache/hadoop/fs/RawLocalFileSystem.java | 22 ---- .../fs/FileContextMainOperationsBaseTest.java | 117 ++++++++++++++---- 7 files changed, 173 insertions(+), 80 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index 09144cb66f9..121e25100b4 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -84,6 +84,9 @@ Trunk (unreleased changes) HADOOP-7202. Improve shell Command base class. (Daryn Sharp via szetszwo) + HADOOP-7223. FileContext createFlag combinations are not clearly defined. + (suresh) + OPTIMIZATIONS BUG FIXES diff --git a/src/java/org/apache/hadoop/fs/ChecksumFs.java b/src/java/org/apache/hadoop/fs/ChecksumFs.java index a55108598be..87cd5a77dec 100644 --- a/src/java/org/apache/hadoop/fs/ChecksumFs.java +++ b/src/java/org/apache/hadoop/fs/ChecksumFs.java @@ -337,8 +337,9 @@ public abstract class ChecksumFs extends FilterFs { int bytesPerSum = fs.getBytesPerSum(); int sumBufferSize = fs.getSumBufferSize(bytesPerSum, bufferSize); this.sums = fs.getRawFs().createInternal(fs.getChecksumFile(file), - EnumSet.of(CreateFlag.OVERWRITE), absolutePermission, sumBufferSize, - replication, blockSize, progress, bytesPerChecksum, createParent); + EnumSet.of(CreateFlag.CREATE, CreateFlag.OVERWRITE), + absolutePermission, sumBufferSize, replication, blockSize, progress, + bytesPerChecksum, createParent); sums.write(CHECKSUM_VERSION, 0, CHECKSUM_VERSION.length); sums.writeInt(bytesPerSum); } diff --git a/src/java/org/apache/hadoop/fs/CreateFlag.java b/src/java/org/apache/hadoop/fs/CreateFlag.java index 375876a8bdb..2f2083c0c88 100644 --- a/src/java/org/apache/hadoop/fs/CreateFlag.java +++ b/src/java/org/apache/hadoop/fs/CreateFlag.java @@ -17,49 +17,63 @@ */ package org.apache.hadoop.fs; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.EnumSet; + +import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.classification.InterfaceAudience; import org.apache.hadoop.classification.InterfaceStability; /**************************************************************** - *CreateFlag specifies the file create semantic. Users can combine flags like:
- * + * CreateFlag specifies the file create semantic. Users can combine flags like:
+ * * EnumSet.of(CreateFlag.CREATE, CreateFlag.APPEND) * - * and pass it to {@link org.apache.hadoop.fs.FileSystem #create(Path f, FsPermission permission, - * EnumSet flag, int bufferSize, short replication, long blockSize, - * Progressable progress)}. - * *

- * Combine {@link #OVERWRITE} with either {@link #CREATE} - * or {@link #APPEND} does the same as only use - * {@link #OVERWRITE}.
- * Combine {@link #CREATE} with {@link #APPEND} has the semantic: + * + * Use the CreateFlag as follows: *

    - *
  1. create the file if it does not exist; - *
  2. append the file if it already exists. + *
  3. CREATE - to create a file if it does not exist, + * else throw FileAlreadyExists.
    4. + *
  4. APPEND - to append to a file if it exists, + * else throw FileNotFoundException.
    5. + *
  5. OVERWRITE - to truncate a file if it exists, + * else throw FileNotFoundException.
    6. + *
  6. CREATE|APPEND - to create a file if it does not exist, + * else append to an existing file.
    7. + *
  7. CREATE|OVERWRITE - to create a file if it does not exist, + * else overwrite an existing file.
    8. + *
+ * + * Following combination is not valid and will result in + * {@link HadoopIllegalArgumentException}: + *
    + *
  1. APPEND|OVERWRITE
    2. + *
  2. CREATE|APPEND|OVERWRITE
    3. *
*****************************************************************/ @InterfaceAudience.Public -@InterfaceStability.Stable +@InterfaceStability.Evolving public enum CreateFlag { /** - * create the file if it does not exist, and throw an IOException if it + * Create a file. See javadoc for more description * already exists */ CREATE((short) 0x01), /** - * create the file if it does not exist, if it exists, overwrite it. + * Truncate/overwrite a file. Same as POSIX O_TRUNC. See javadoc for description. */ OVERWRITE((short) 0x02), /** - * append to a file, and throw an IOException if it does not exist + * Append to a file. See javadoc for more description. */ APPEND((short) 0x04); - private short mode; + private final short mode; private CreateFlag(short mode) { this.mode = mode; @@ -68,4 +82,38 @@ public enum CreateFlag { short getMode() { return mode; } + + /** + * Validate the CreateFlag for create operation + * @param path Object representing the path; usually String or {@link Path} + * @param pathExists pass true if the path exists in the file system + * @param flag set of CreateFlag + * @throws IOException on error + * @throws HadoopIllegalArgumentException if the CreateFlag is invalid + */ + public static void validate(Object path, boolean pathExists, + EnumSet flag) throws IOException { + if (flag == null || flag.isEmpty()) { + throw new HadoopIllegalArgumentException(flag + + " does not specify any options"); + } + final boolean append = flag.contains(APPEND); + final boolean overwrite = flag.contains(OVERWRITE); + + // Both append and overwrite is an error + if (append && overwrite) { + throw new HadoopIllegalArgumentException( + flag + "Both append and overwrite options cannot be enabled."); + } + if (pathExists) { + if (!(append || overwrite)) { + throw new FileAlreadyExistsException("File already exists: " + + path.toString() + + ". Append or overwrite option must be specified in " + flag); + } + } else if (!flag.contains(CREATE)) { + throw new FileNotFoundException("Non existing file: " + path.toString() + + ". Create option is not specified in " + flag); + } + } } \ No newline at end of file diff --git a/src/java/org/apache/hadoop/fs/FileContext.java b/src/java/org/apache/hadoop/fs/FileContext.java index 5db4e61f208..c10f83797aa 100644 --- a/src/java/org/apache/hadoop/fs/FileContext.java +++ b/src/java/org/apache/hadoop/fs/FileContext.java @@ -509,7 +509,7 @@ public final class FileContext { * writing into the file. * * @param f the file name to open - * @param createFlag gives the semantics of create: overwrite, append etc. + * @param createFlag gives the semantics of create; see {@link CreateFlag} * @param opts file creation options; see {@link Options.CreateOpts}. *
    *
  • Progress - to report progress on the operation - default null @@ -2055,7 +2055,10 @@ public final class FileContext { OutputStream out = null; try { in = open(qSrc); - out = create(qDst, EnumSet.of(CreateFlag.OVERWRITE)); + EnumSet createFlag = overwrite ? EnumSet.of( + CreateFlag.CREATE, CreateFlag.OVERWRITE) : + EnumSet.of(CreateFlag.CREATE); + out = create(qDst, createFlag); IOUtils.copyBytes(in, out, conf, true); } catch (IOException e) { IOUtils.closeStream(out); diff --git a/src/java/org/apache/hadoop/fs/FileSystem.java b/src/java/org/apache/hadoop/fs/FileSystem.java index 74ccefb69cf..6e80c7d4204 100644 --- a/src/java/org/apache/hadoop/fs/FileSystem.java +++ b/src/java/org/apache/hadoop/fs/FileSystem.java @@ -699,24 +699,21 @@ public abstract class FileSystem extends Configured implements Closeable { FsPermission absolutePermission, EnumSet flag, int bufferSize, short replication, long blockSize, Progressable progress, int bytesPerChecksum) throws IOException { + + boolean pathExists = exists(f); + CreateFlag.validate(f, pathExists, flag); // Default impl assumes that permissions do not matter and // nor does the bytesPerChecksum hence // calling the regular create is good enough. // FSs that implement permissions should override this. - if (exists(f)) { - if (flag.contains(CreateFlag.APPEND)) { - return append(f, bufferSize, progress); - } else if (!flag.contains(CreateFlag.OVERWRITE)) { - throw new IOException("File already exists: " + f); - } - } else { - if (flag.contains(CreateFlag.APPEND) && !flag.contains(CreateFlag.CREATE)) - throw new IOException("File already exists: " + f.toString()); + if (pathExists && flag.contains(CreateFlag.APPEND)) { + return append(f, bufferSize, progress); } - return this.create(f, absolutePermission, flag.contains(CreateFlag.OVERWRITE), bufferSize, replication, + return this.create(f, absolutePermission, + flag.contains(CreateFlag.OVERWRITE), bufferSize, replication, blockSize, progress); } diff --git a/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java b/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java index 18ef152baf3..3a54fae955b 100644 --- a/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java +++ b/src/java/org/apache/hadoop/fs/RawLocalFileSystem.java @@ -264,28 +264,6 @@ public class RawLocalFileSystem extends FileSystem { return out; } - - @Override - protected FSDataOutputStream primitiveCreate(Path f, - FsPermission absolutePermission, EnumSet flag, - int bufferSize, short replication, long blockSize, Progressable progress, - int bytesPerChecksum) throws IOException { - - if(flag.contains(CreateFlag.APPEND)){ - if (!exists(f)){ - if(flag.contains(CreateFlag.CREATE)) { - return create(f, false, bufferSize, replication, blockSize, null); - } - } - return append(f, bufferSize, null); - } - - FSDataOutputStream out = create(f, flag.contains(CreateFlag.OVERWRITE), - bufferSize, replication, blockSize, progress); - setPermission(f, absolutePermission); - return out; - } - public boolean rename(Path src, Path dst) throws IOException { if (pathToFile(src).renameTo(pathToFile(dst))) { return true; diff --git a/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java b/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java index 1af5a6391bc..60229c050ce 100644 --- a/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java +++ b/src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java @@ -21,8 +21,8 @@ package org.apache.hadoop.fs; import java.io.FileNotFoundException; import java.io.IOException; import java.util.EnumSet; -import java.util.Iterator; +import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.fs.Options.CreateOpts; import org.apache.hadoop.fs.Options.Rename; import org.apache.hadoop.fs.permission.FsPermission; @@ -32,6 +32,7 @@ import org.junit.Before; import org.junit.Test; import static org.apache.hadoop.fs.FileContextTestHelper.*; +import static org.apache.hadoop.fs.CreateFlag.*; /** *

    @@ -155,7 +156,7 @@ public abstract class FileContextMainOperationsBaseTest { // Now open a file relative to the wd we just set above. Path absolutePath = new Path(absoluteDir, "foo"); - fc.create(absolutePath, EnumSet.of(CreateFlag.CREATE)).close(); + fc.create(absolutePath, EnumSet.of(CREATE)).close(); fc.open(new Path("foo")).close(); @@ -645,7 +646,7 @@ public abstract class FileContextMainOperationsBaseTest { fc.mkdir(path.getParent(), FsPermission.getDefault(), true); - FSDataOutputStream out = fc.create(path, EnumSet.of(CreateFlag.CREATE), + FSDataOutputStream out = fc.create(path, EnumSet.of(CREATE), CreateOpts.repFac((short) 1), CreateOpts .blockSize(getDefaultBlockSize())); out.write(data, 0, len); @@ -670,31 +671,93 @@ public abstract class FileContextMainOperationsBaseTest { } + @Test(expected=HadoopIllegalArgumentException.class) + public void testNullCreateFlag() throws IOException { + Path p = getTestRootPath(fc, "test/file"); + fc.create(p, null); + Assert.fail("Excepted exception not thrown"); + } + + @Test(expected=HadoopIllegalArgumentException.class) + public void testEmptyCreateFlag() throws IOException { + Path p = getTestRootPath(fc, "test/file"); + fc.create(p, EnumSet.noneOf(CreateFlag.class)); + Assert.fail("Excepted exception not thrown"); + } + + @Test(expected=FileAlreadyExistsException.class) + public void testCreateFlagCreateExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/file"); + createFile(p); + fc.create(p, EnumSet.of(CREATE)); + Assert.fail("Excepted exception not thrown"); + } + + @Test(expected=FileNotFoundException.class) + public void testCreateFlagOverwriteNonExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + fc.create(p, EnumSet.of(OVERWRITE)); + Assert.fail("Excepted exception not thrown"); + } + @Test - public void testOverwrite() throws IOException { - Path path = getTestRootPath(fc, "test/hadoop/file"); - - fc.mkdir(path.getParent(), FsPermission.getDefault(), true); - - createFile(path); - - Assert.assertTrue("Exists", exists(fc, path)); - Assert.assertEquals("Length", data.length, fc.getFileStatus(path).getLen()); - - try { - fc.create(path, EnumSet.of(CreateFlag.CREATE)); - Assert.fail("Should throw IOException."); - } catch (IOException e) { - // Expected - } - - FSDataOutputStream out = fc.create(path,EnumSet.of(CreateFlag.OVERWRITE)); + public void testCreateFlagOverwriteExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/file"); + createFile(p); + FSDataOutputStream out = fc.create(p, EnumSet.of(OVERWRITE)); + writeData(fc, p, out, data, data.length); + } + + @Test(expected=FileNotFoundException.class) + public void testCreateFlagAppendNonExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + fc.create(p, EnumSet.of(APPEND)); + Assert.fail("Excepted exception not thrown"); + } + + @Test + public void testCreateFlagAppendExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/file"); + createFile(p); + FSDataOutputStream out = fc.create(p, EnumSet.of(APPEND)); + writeData(fc, p, out, data, 2 * data.length); + } + + @Test + public void testCreateFlagCreateAppendNonExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + FSDataOutputStream out = fc.create(p, EnumSet.of(CREATE, APPEND)); + writeData(fc, p, out, data, data.length); + } + + @Test + public void testCreateFlagCreateAppendExistingFile() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + createFile(p); + FSDataOutputStream out = fc.create(p, EnumSet.of(CREATE, APPEND)); + writeData(fc, p, out, data, 2*data.length); + } + + @Test(expected=HadoopIllegalArgumentException.class) + public void testCreateFlagAppendOverwrite() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + fc.create(p, EnumSet.of(APPEND, OVERWRITE)); + Assert.fail("Excepted exception not thrown"); + } + + @Test(expected=HadoopIllegalArgumentException.class) + public void testCreateFlagAppendCreateOverwrite() throws IOException { + Path p = getTestRootPath(fc, "test/nonExistent"); + fc.create(p, EnumSet.of(CREATE, APPEND, OVERWRITE)); + Assert.fail("Excepted exception not thrown"); + } + + private static void writeData(FileContext fc, Path p, FSDataOutputStream out, + byte[] data, long expectedLen) throws IOException { out.write(data, 0, data.length); out.close(); - - Assert.assertTrue("Exists", exists(fc, path)); - Assert.assertEquals("Length", data.length, fc.getFileStatus(path).getLen()); - + Assert.assertTrue("Exists", exists(fc, p)); + Assert.assertEquals("Length", expectedLen, fc.getFileStatus(p).getLen()); } @Test @@ -1057,7 +1120,7 @@ public abstract class FileContextMainOperationsBaseTest { //HADOOP-4760 according to Closeable#close() closing already-closed //streams should have no effect. Path src = getTestRootPath(fc, "test/hadoop/file"); - FSDataOutputStream out = fc.create(src, EnumSet.of(CreateFlag.CREATE), + FSDataOutputStream out = fc.create(src, EnumSet.of(CREATE), Options.CreateOpts.createParent()); out.writeChar('H'); //write some data @@ -1091,7 +1154,7 @@ public abstract class FileContextMainOperationsBaseTest { } protected void createFile(Path path) throws IOException { - FSDataOutputStream out = fc.create(path, EnumSet.of(CreateFlag.CREATE), + FSDataOutputStream out = fc.create(path, EnumSet.of(CREATE), Options.CreateOpts.createParent()); out.write(data, 0, data.length); out.close();