HADOOP-11452 make rename/3 public #743

steveloughran · 2019-04-15T19:02:21Z

PR with the previous patches.

This is a WiP but ready for some iniital review; lacks tests & spec.

Also, because the base FileSystem.rename/3 does its own src/dest checks, it's less efficient against object stores. They need their own high-perf subclass.

Change-Id: I1586ef2290d7a3d2d33b1a32e2f0999b07c26143

bgaborg · 2019-04-15T22:40:08Z

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileSystem.java

   * @throws FileAlreadyExistsException dest path exists and is a file
   * @throws ParentNotDirectoryException if the parent path of dest is not
   * a directory
   * @throws IOException on failure
   */
-  @Deprecated


You remove the deprecation here from the abstract FS. How will it affect the target release it can get in?
FS is

@InterfaceAudience.Public @InterfaceStability.Stable

Also it changed to protected to public. The same question applies.

that's exactly the purpose: take a method which is protected and used only by FileContext, and make it public. It's been in for a while, been stable -and now needs tests and documentation of what it really does

hadoop-yetus · 2019-04-16T02:45:21Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

 deletion, preventing the stores' use as drop-in replacements for HDFS.

 ### `boolean rename(Path src, Path d)`

 In terms of its specification, `rename()` is one of the most complex operations within a filesystem .

 In terms of its implementation, it is the one with the most ambiguity regarding when to return false
-versus raising an exception.
+versus raising an exception. This makes less useful to use than
+others: in production code it is usually wrapped by code to raise an 


whitespace:end of line

hadoop-yetus · 2019-04-16T02:45:22Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+The semantics of this method are simpler and more clearly defined.
+
+- The final path of the rename is always that of the `dest` argument; there
+is no attempt to generate a new path underneath a target directory. 


whitespace:end of line

hadoop-yetus · 2019-04-16T02:45:23Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+another, SHOULD be atomic. Many applications rely on this as a way to commit operations.
+
+* However, the base implementation is *not* atomic; some of the precondition checks
+are performed separately. 


whitespace:end of line

steveloughran · 2019-04-16T06:33:14Z

Pushed up new version which does a lot more w.r.t testing, including updating FSMainOperationsBaseTest to java7/8 and implementing an S3A subclass (which fails, see below)
Also adds an override point to make it possible for FSs to implement their exception-raising rename after the common semantics checks.

I'm not comfortable with that design; I think it'd be better to pull the rename preconditions into some standalone class which takes a src & dest FS status and path strings, and do the checks there where they can validated in isolation. S3A would just precalculate those and then reuse them later on. Be easier for testing too.

but...that doesn't help abfs so much, does it?

[ERROR] testWorkingDirectory(org.apache.hadoop.fs.contract.s3a.ITestS3AFSMainOperations)  Time elapsed: 3.305 s  <<< FAILURE!
java.lang.AssertionError: expected:<s3a://hwdev-steve-new/ITestS3AFSMainOperations/test> but was:<>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at org.apache.hadoop.fs.FSMainOperationsBaseTest.testWorkingDirectory(FSMainOperationsBaseTest.java:162)

[ERROR] testCopyToLocalWithUseRawLocalFileSystemOption(org.apache.hadoop.fs.contract.s3a.ITestS3AFSMainOperations)  Time elapsed: 1.291 s  <<< ERROR!
java.io.IOException: Mkdirs failed to create /ITestS3AFSMainOperations
        at org.apache.hadoop.fs.RawLocalFileSystem.create(RawLocalFileSystem.java:317)
        at org.apache.hadoop.fs.RawLocalFileSystem.create(RawLocalFileSystem.java:305)
        at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:1111)
        at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:1000)
        at org.apache.hadoop.fs.FileSystem.create(FileSystem.java:988)
        at org.apache.hadoop.fs.FSMainOperationsBaseTest.writeFile(FSMainOperationsBaseTest.java:1042)
        at org.apache.hadoop.fs.FSMainOperationsBaseTest.testCopyToLocalWithUseRawLocalFileSystemOption(FSMainOperationsBaseTest.java:1033)

steveloughran · 2019-04-16T06:34:33Z

that new override point is what breaks the harfs test

hadoop-yetus · 2019-04-16T10:32:11Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

 deletion, preventing the stores' use as drop-in replacements for HDFS.

 ### `boolean rename(Path src, Path d)`

 In terms of its specification, `rename()` is one of the most complex operations within a filesystem .

 In terms of its implementation, it is the one with the most ambiguity regarding when to return false
-versus raising an exception.
+versus raising an exception. This makes less useful to use than
+others: in production code it is usually wrapped by code to raise an 


whitespace:end of line

hadoop-yetus · 2019-04-16T10:32:13Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+The semantics of this method are simpler and more clearly defined.
+
+- The final path of the rename is always that of the `dest` argument; there
+is no attempt to generate a new path underneath a target directory. 


whitespace:end of line

hadoop-yetus · 2019-04-16T10:32:15Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+another, SHOULD be atomic. Many applications rely on this as a way to commit operations.
+
+* However, the base implementation is *not* atomic; some of the precondition checks
+are performed separately. 


whitespace:end of line

hadoop-yetus · 2019-04-30T02:01:14Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

 deletion, preventing the stores' use as drop-in replacements for HDFS.

 ### `boolean rename(Path src, Path d)`

 In terms of its specification, `rename()` is one of the most complex operations within a filesystem .

 In terms of its implementation, it is the one with the most ambiguity regarding when to return false
-versus raising an exception.
+versus raising an exception. This makes less useful to use than
+others: in production code it is usually wrapped by code to raise an 


whitespace:end of line

hadoop-yetus · 2019-04-30T02:01:16Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+The semantics of this method are simpler and more clearly defined.
+
+- The final path of the rename is always that of the `dest` argument; there
+is no attempt to generate a new path underneath a target directory. 


whitespace:end of line

hadoop-yetus · 2019-04-30T02:01:17Z

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

+another, SHOULD be atomic. Many applications rely on this as a way to commit operations.
+
+* However, the base implementation is *not* atomic; some of the precondition checks
+are performed separately. 


whitespace:end of line

DadanielZ · 2019-05-01T04:45:22Z

...ommon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java

-      Assert.fail("Should throw IOException");
-    } catch (IOException ioe) {
-      // expected
+      intercept(IOException.class, () ->


This test does permission check for getFileStatus call.
But according to HdfsPermissionsGuide, getFileStatus/getFileInfo call should not check the permission. I'm wondering do I miss any documents?

I'm going to revert all changes to this test suite other than those which go near rename, so whatever is tested for today is going to be exactly what is continued to be tested for. I Don't know what the nuances of HDFS permissions are.
I'd assume that the getFileStatus call only needs metadata/list permissions, rather than read access. That HDFS doc states that client needs execute permission on all parent directories, so that it can get from / to the file. Therefore the "has permissions to call getFileXXX" is implicit if you can get to the dir, right?

DadanielZ · 2019-05-01T04:49:01Z

...ommon-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java

-      Assert.fail("Should throw IOException");
-    } catch (IOException ioe) {
-      // expected
+      intercept(IOException.class, () ->


Same here, I think there should not be permission check for listStatus according to HdfsPermissionsGuide

Now that's interesting. I wonder what unix does there. Because HDFS should really be doing the same. If I'm in dir /a and don't have exec perms for /a/b, I should be able call stat /a/b, just not ls(/a/b). (pauses to check. OK, on the command line, removing x perms from a dir means ls() returns an empty list, not an error

(and hadoop fs -ls -R command returns an empty list too)

ehiggs · 2019-06-18T08:27:33Z

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java

+      Path dst,
+      final Options.Rename... options)
+      throws IOException {
+      fs.rename(src, dst, options);


The deprecated version checks the return code here and bails if the rename wasn't successful. If Options includes Options.Rename.NONE and dst already exists, this will return false? If so, we shouldn't proceed to overwrite the checksums.

if options = NONE and the dest exists, the call MUST raise an exception. I've just reviewed this carefully and added comments to clarify

ehiggs · 2019-06-18T08:30:51Z

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ChecksumFileSystem.java

+      Path srcCheckFile = getChecksumFile(src);
+      Path dstCheckFile = getChecksumFile(dst);
+      if (fs.exists(srcCheckFile)) { //try to rename checksum
+        fs.rename(srcCheckFile, dstCheckFile, Options.Rename.OVERWRITE);


Should this be options with Options.Rename.OVERWRITE appended? I think all the Rename options work alone, but if we are passing an unbounded list of options, not passing them through smells like a bug.

interesting thought. The only current option is trash; don't know if we should retain that. I will add a comment in the javadocs of Options.Rename on this topic

+added comments in Option as well as Checksum FS

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSExceptionMessages.java

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/RenameHelper.java

steveloughran · 2019-09-11T20:09:37Z

for commenters, yes, this needs a rebase against trunk and then a review of all your comments. I cherish your patience, especially as all the comments are good ones

…TaskApplication in JobPlanner Author: Prateek Maheshwari <pmaheshwari@apache.org> Reviewers: Ray Mathuru <rmathuru@linkedin.com>, Jagadish Venkatraman <vjagadish1989@linkedin.com>, Sanil Jain <snjain@linkedin.com> Closes apache#743 from prateekm/app-mode-fix

PR with the previous patches. This is a WiP but ready for some iniital review; lacks tests & spec. Also, because the base FileSystem.rename/3 does its own src/dest checks, it's less efficient against object stores. They need their own high-perf subclass. Change-Id: I1586ef2290d7a3d2d33b1a32e2f0999b07c26143

+ updates docs + contains fix for HADOOP-16255 : checksum FS doesn't do rename/3 properly tests are based on those of rename; there's still some stuff about renaming into an empty directory which makes me suspect there's ambiguity there Change-Id: Ic1ca6cbe3e9ca80ab8e1459167a7012678e856fc

* exceptions match expectations of FSMainOperationsBaseTest * clean up FSMainOperationsBaseTest by moving to intercept, try with resources. * S3A to implement subclass of FSMainOperationsBaseTest, ITestS3AFSMainOperations * S3A to implement ITestS3AContractRenameEx * Add protected override point, executeInnerRename, for implementing rename/3; base class calls rename() and throws if that returns false, i.e. current behaviour * S3A overrides executeInnerRename to call its own innerRename() and so raise all failures as IOEs. Issues: is the rename point executeInnerRename() a good name? S3AFS should really implement a direct rename/3 so there's no duplication of checks for parent etc, maybe even passing the values down. Or we make sure innerRename() is consistent with the spec, which primarily means logic about dest dir existing. Change-Id: I0ac3695434d85072ab860854e5e88bc6d36e754a

Change-Id: If2ab67152e08e4c2a225f6e89c24a5d1ff79ee59

@Retry

Factored the rename check logic out into a RenameHelper which is then used in S3A FileSystem as the PoC of how it can use the RenameHelper then directly invoke the inner operations. Added some more @Retry attributes as appropriate. Conclusion: it works, but for efficient IOPS then `innerRename()` needs to take optional source and dest status values and so so omit repeating the checks. For more work on this * the tests; file context has some so review and add to AbstractContractRenameTest. Also, based on some feedback from Sean Mackrory: verify the renamed files actually have the data. * move internal use of rename (distcp, maprv2 FileOutputcommitter), etc to use this. of the 50+ places which call rename, they seem split 3-ways int 1. subclasses and forwarding 2. invocations whch check the return value and throw an IOE 3. invocations which are written on the assumption that renames raise exceptions on failure 2 & 3 are the ones to change. Change-Id: Id77ed7352b9d5ddb124f9191c5c5f1b8a76da7bb

Review of RenameHelper based on current coding styles and plans (IOStats, etc) Change-Id: I3d39ee3ed04a10e7db2c2b2c79833b945b4d691b

S3A high performance edition. This avoids all surplus S3 calls and has meaningful exception raising. TODO: * pull the S3A code out into is own operation + extra callbacks (innerGetFileStatus is all that's needed) * see if the FileContext default logic can be pulled out too, using a custom set of callbacks. If it can't the logic is broken. * do some testing Change-Id: I408b2cfe93f266cf0c9084fa8f05bb84b65c2bad

* Add RawLocalFileSystem rename with useful errors * pull out all rename docs into their own filesystem.md doc * Add a callback impl which => FileContext too, at least for the nonatomic rename. FC doesn't do path name checking. Should we? Proposed changes * move the new interfaces up to o.a.h.fs, so that .impl is never imported in FileSystem APIS. * remove the createRename callbacks method, just have stores with implement rename/3 other than the base FS to override all of rename 3. Change-Id: I1fab598553b8e9de4d659b80248bac440dbac018

steveloughran · 2020-09-14T12:08:23Z

as per Daryn's comments in : https://issues.apache.org/jira/browse/HADOOP-16281?focusedCommentId=16832628&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16832628

No changes to HDFS error messages. Review this

steveloughran · 2021-03-02T15:24:12Z

closing and creating a new rebased/squashed PR. Still need to look at the comments related to permissions

steveloughran added the work-in-progress label Apr 15, 2019

bgaborg reviewed Apr 15, 2019

View reviewed changes

hadoop-yetus reviewed Apr 16, 2019

View reviewed changes

steveloughran added the fs/s3 changes related to hadoop-aws; submitter must declare test endpoint label Apr 16, 2019

hadoop-yetus reviewed Apr 16, 2019

View reviewed changes

hadoop-yetus reviewed Apr 30, 2019

View reviewed changes

DadanielZ reviewed May 1, 2019

View reviewed changes

ehiggs reviewed Jun 18, 2019

View reviewed changes

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FSExceptionMessages.java Outdated Show resolved Hide resolved

ehiggs reviewed Jun 18, 2019

View reviewed changes

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/RenameHelper.java Outdated Show resolved Hide resolved

steveloughran removed the work-in-progress label Jul 9, 2019

apache deleted a comment from hadoop-yetus Sep 11, 2019

steveloughran added 4 commits August 29, 2020 13:34

HADOOP-11452 trying to move rename/3 logic into its own class

78ce317

Change-Id: If2ab67152e08e4c2a225f6e89c24a5d1ff79ee59

steveloughran and others added 2 commits August 29, 2020 13:59

HADOOP-11452. Rename

f4422a4

Review of RenameHelper based on current coding styles and plans (IOStats, etc) Change-Id: I3d39ee3ed04a10e7db2c2b2c79833b945b4d691b

steveloughran force-pushed the filesystem/HADOOP-11452-rename branch from 2426328 to f4422a4 Compare August 29, 2020 13:39

steveloughran added 2 commits August 30, 2020 13:51

apache deleted a comment from hadoop-yetus Sep 1, 2020

steveloughran mentioned this pull request Sep 1, 2020

HDFS-15484. Add new method batchRename for DistributedFileSystem and W… #2235

Open

apache deleted a comment from hadoop-yetus Sep 10, 2020

steveloughran mentioned this pull request Sep 18, 2020

HADOOP-15136. Correct typos in filesystem.md #2314

Merged

apache deleted a comment from hadoop-yetus Sep 21, 2020

steveloughran mentioned this pull request Sep 21, 2020

HADOOP-17255. JavaKeyStoreProvider fails to create a new key if the keystore is HDFS #2291

Merged

steveloughran closed this Mar 2, 2021

steveloughran deleted the filesystem/HADOOP-11452-rename branch March 2, 2021 15:24

steveloughran mentioned this pull request Mar 2, 2021

HADOOP-11452 make rename/3 public #2735

Closed

HADOOP-11452 make rename/3 public #743

HADOOP-11452 make rename/3 public #743

Uh oh!

Conversation

steveloughran commented Apr 15, 2019

Uh oh!

bgaborg Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran commented Apr 16, 2019

Uh oh!

steveloughran commented Apr 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehiggs Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveloughran commented Sep 11, 2019

Uh oh!

steveloughran commented Sep 14, 2020

Uh oh!

steveloughran commented Mar 2, 2021

Uh oh!

Uh oh!

bgaborg Apr 15, 2019 •

edited

Loading

ehiggs Jun 18, 2019 •

edited

Loading