-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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.
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
HADOOP-18117. Add an option to preserve root directory permissions #3970
Conversation
🎊 +1 overall
This message was automatically generated. |
It might be better to add a test, e.g. ...
fs.setOwner(src,"userA", "groupA");
fs.setTimes(src, new Random().nextLong(), new Random().nextLong());
String[] args = {"-p", "-update", src.toString(), dest.toString()};
DistCp distCp = new DistCp(conf, null);
assertEquals(DistCpConstants.SUCCESS, ToolRunner.run(conf, distCp, args));
FileStatus srcStatus = fs.getFileStatus(src);
FileStatus destStatus = fs.getFileStatus(dest);
assertTrue(destStatus.getOwner().equals(srcStatus.getOwner()));
assertTrue(destStatus.getModificationTime() == srcStatus.getModificationTime()); |
@whbing you mean something like this to test functionality before and after? @Test
public void testUpdateRootDirectoryAttributes() throws Exception {
FileSystem fs = cluster.getFileSystem();
Path source = new Path("/src");
Path dest1 = new Path("/dest1");
Path dest2 = new Path("/dest2");
fs.delete(source, true);
fs.delete(dest1, true);
fs.delete(dest2, true);
// Create a source dir
fs.mkdirs(source);
fs.setOwner(source,"userA", "groupA");
fs.setTimes(source, new Random().nextLong(), new Random().nextLong());
GenericTestUtils.createFiles(fs, source, 3, 5, 5);
// should not preserve attrs
DistCpTestUtils.assertRunDistCp(DistCpConstants.SUCCESS, source.toString(),
dest1.toString(), "-p -update", conf);
FileStatus srcStatus = fs.getFileStatus(source);
FileStatus destStatus1 = fs.getFileStatus(dest1);
assertNotEquals(destStatus1.getOwner(), srcStatus.getOwner());
assertNotEquals(destStatus1.getModificationTime(), srcStatus.getModificationTime());
// should preserve attrs
DistCpTestUtils.assertRunDistCp(DistCpConstants.SUCCESS, source.toString(),
dest2.toString(), "-p -update -updateRootDirectoryAttributes", conf);
FileStatus destStatus2 = fs.getFileStatus(dest1);
assertEquals(destStatus2.getOwner(), srcStatus.getOwner());
assertEquals(destStatus2.getModificationTime(), srcStatus.getModificationTime());
} |
🎊 +1 overall
This message was automatically generated. |
@mohan3d Overall it looks good. Please check the checkstyle reported by Yetus. |
🎊 +1 overall
This message was automatically generated. |
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/TestDistCpSystem.java
Outdated
Show resolved
Hide resolved
@@ -363,6 +363,7 @@ Command Line Options | |||
| `-xtrack <path>` | Save information about missing source files to the specified path. | This option is only valid with `-update` option. This is an experimental property and it cannot be used with `-atomic` option. | | |||
| `-direct` | Write directly to destination paths | Useful for avoiding potentially very expensive temporary file rename operations when the destination is an object store | | |||
| `-useiterator` | Uses single threaded listStatusIterator to build listing | Useful for saving memory at the client side. Using this option will ignore the numListstatusThreads option | | |||
| `-updateRootDirectoryAttributes` | Update root directory attributes (eg permissions, ownership ...) | Useful if you need to enforce root directory attributes update when using distcp | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you replace updateRoot instead of updateRootDirectoryAttributes or give another short name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, since we have no other suggestions I will go with updateRoot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ferhui should I re-work the internal code so everything is updateRoot
instead of updateRootDirectoryAttributes
or only the enduser distcp
tool?
Better to be consistent and use updateRoot everywhere.
a79999c
to
f62d3c3
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Since there are no other comments within these days, I merge this. |
Thanks @ferhui! |
Description of PR
Add an option to preserve root directory permissions when using distcp
How was this patch tested?
dev-support/bin/test-patch against trunk and unittest
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?