steveloughran commented on a change in pull request #3101:
URL: https://github.com/apache/hadoop/pull/3101#discussion_r650192177
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -3815,36 +3815,52 @@ public void copyFromLocalFile(boolean delSrc, boolean
overwrite, Path src,
@Retries.RetryTranslated
private void innerCopyFromLocalFile(boolean delSrc, boolean overwrite,
Path src, Path dst)
- throws IOException, FileAlreadyExistsException, AmazonClientException {
+ throws IOException, PathExistsException, AmazonClientException {
LOG.debug("Copying local file from {} to {}", src, dst);
// Since we have a local file, we don't need to stream into a temporary
file
LocalFileSystem local = getLocal(getConf());
- File srcfile = local.pathToFile(src);
- if (!srcfile.exists()) {
+ File srcFile = local.pathToFile(src);
+ if (!srcFile.exists()) {
throw new FileNotFoundException("No file: " + src);
}
- if (!srcfile.isFile()) {
- throw new FileNotFoundException("Not a file: " + src);
- }
try {
- FileStatus status = innerGetFileStatus(dst, false, StatusProbeEnum.ALL);
- if (!status.isFile()) {
- throw new FileAlreadyExistsException(dst + " exists and is not a
file");
+ S3AFileStatus dstStatus = innerGetFileStatus(dst, false,
StatusProbeEnum.ALL);
+ if (srcFile.isFile() && dstStatus.isDirectory()) {
+ throw new PathExistsException("Source is file and destination '" + dst
+ "' is directory");
Review comment:
should include source too; and I suspect the line will need cutting down
to be ~= 80 chars wide. (yes, that's very dated; chosen for ease of
side-by-side review and all discussion about changing it never ends up well)
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ACopyFromLocalFile.java
##########
@@ -158,4 +191,11 @@ public File createTempFile(String text) throws IOException
{
FileUtils.write(f, text, ASCII);
return f;
}
+
+ private void assertPathExists(String message,
Review comment:
give it a different name, like assertNioPathExists
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3ACopyFromLocalFile.java
##########
@@ -120,15 +120,48 @@ public void testCopyMissingFile() throws Throwable {
() -> upload(file, true));
}
+ /*
+ * The following path is being created on disk and copied over
+ * /parent/ (trailing slash to make it clear it's a directory
+ * /parent/test1.txt
+ * /parent/child/test.txt
+ */
@Test
- @Ignore("HADOOP-15932")
- public void testCopyDirectoryFile() throws Throwable {
- file = File.createTempFile("test", ".txt");
- // first upload to create
- intercept(FileNotFoundException.class, "Not a file",
- () -> upload(file.getParentFile(), true));
+ public void testCopyTreeDirectoryWithoutDelete() throws Throwable {
+ java.nio.file.Path srcDir = Files.createTempDirectory("parent");
Review comment:
There's nothing here which verifies that the dest tree matches the
source, including contents.
It needs something like `testCopyFile()` where the byte value of the two
.txt files are compared with the values at the far end.
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
##########
@@ -3815,36 +3815,52 @@ public void copyFromLocalFile(boolean delSrc, boolean
overwrite, Path src,
@Retries.RetryTranslated
private void innerCopyFromLocalFile(boolean delSrc, boolean overwrite,
Path src, Path dst)
- throws IOException, FileAlreadyExistsException, AmazonClientException {
+ throws IOException, PathExistsException, AmazonClientException {
LOG.debug("Copying local file from {} to {}", src, dst);
// Since we have a local file, we don't need to stream into a temporary
file
LocalFileSystem local = getLocal(getConf());
- File srcfile = local.pathToFile(src);
- if (!srcfile.exists()) {
+ File srcFile = local.pathToFile(src);
+ if (!srcFile.exists()) {
throw new FileNotFoundException("No file: " + src);
}
- if (!srcfile.isFile()) {
- throw new FileNotFoundException("Not a file: " + src);
- }
try {
- FileStatus status = innerGetFileStatus(dst, false, StatusProbeEnum.ALL);
- if (!status.isFile()) {
- throw new FileAlreadyExistsException(dst + " exists and is not a
file");
+ S3AFileStatus dstStatus = innerGetFileStatus(dst, false,
StatusProbeEnum.ALL);
+ if (srcFile.isFile() && dstStatus.isDirectory()) {
+ throw new PathExistsException("Source is file and destination '" + dst
+ "' is directory");
}
+
if (!overwrite) {
- throw new FileAlreadyExistsException(dst + " already exists");
+ throw new PathExistsException(dst + " already exists");
}
} catch (FileNotFoundException e) {
// no destination, all is well
}
- final String key = pathToKey(dst);
- final ObjectMetadata om = newObjectMetadata(srcfile.length());
- Progressable progress = null;
- PutObjectRequest putObjectRequest = newPutObjectRequest(key, om, srcfile);
- invoker.retry("copyFromLocalFile(" + src + ")", dst.toString(), true,
- () -> executePut(putObjectRequest, progress));
+
+ if (srcFile.isDirectory()) {
+ // make the directory in the destination
+ String key = pathToKey(dst);
+ createFakeDirectory(key);
Review comment:
I think we'd better of with a full `mkdirs` operation. That goes up the
tree and makes sure that there's no files at that location, above etc. To keep
things first, only the top path needs amkdirs; after that we know all is good.
And don't bother with needless markers either. Of course, we can't call
mkdirs() as it is now an audit span and it would lose the outer. so you'll have
to create/invoke a new MkdirsOperation here.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
