[
https://issues.apache.org/jira/browse/HADOOP-14600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16171960#comment-16171960
]
Steve Loughran commented on HADOOP-14600:
-----------------------------------------
I like where this is going, done another review. Without looking at my first
review...sorry if I'm now inconsistent.
h3. production code
* I'm hoping other people with native build setups can play with this. Any
volunteers watching?
* {{FsPermission}}: wrap the new octal mask table with <pre></pre> tags for
formatting in javadocs; document {{mode}} param.
h3. {{NativeIO}}
* L224, L434, anywhere else (TestFsPermission) add a "." at the end of the
first javadoc sentence in the javadocs. Java 8 javadoc expects that.
* L455, Raise a {{PathIOException}} with the path, exception string to include
the value of nioe.toString. e.g {{throw new PathIOException(path,
nioe.toString()), nioe)}}
h3. {{NativeIO.c}}
* Where is the windows API call {{FindFileOwnerAndPermission}} coming from? I
couldn't find it on MSDN.
h3. {{RawLocalFileSystem}}
* L700: don't bother with the HADOOP- ref in the comments; we use the git logs
for that.
* but do use javadocs instead.
* L707 use {{NativeIO.isAvailable()}} as the check for it being present.
* {{loadPermissionByNativeIO}}. Have it rethrow as an IOE & catch that in
loadPermissionInfo() at L771.
One thing to consider is: if the native IO fails for one instance, is it going
to stay broken for everything? That is: after 1 failure, set some static
variable so there are no further attempts? I think this is probably
over-complex for now, and so just leave as is.
h3. TestNativeIO
* in the finally clauses, {{fileSystem != null && path != null}} can never be
false. Use {{ContractTestUtils.cleanup()}} for a more resilient cleanup.
* {{doStatTest}} for all the tests which are assertTrue/False/Not null, include
a text message which is meanginful. Maybe include the stat.toString() value
* {{testStatOnError}} Use {{LambdaTestUtils.intercept}} for better reporting
when an exception isn't raised, checking of the response, etc.
{code}
LambdaTestUtils.intercept(IOException.class, testFilePath,
() -> NativeIO.POSIX.getStat(testFilePath));
{code}
{{testMultiThreadedStat()}}. It's good to see some work here, we (I!) undertest
the concurrency.
* could you use Futures & Callable? That has exception raising built in. Or,
if you must use Thread, give it a java 8 closure rather than a subclassing.
One thing to consdier is could
{{DeprecatedRawLocalFileStatus.loadPermissionInfoByNonNativeIO()}} could be
made package scoped, tagged @VisibleForTesting and then used explicitly in
{{TestNativeIO.doStatTest()}}? That'd remove the stat command buildup there,
and verify that the output of the native and non-native stat calls match
exactly,.
> LocatedFileStatus constructor forces RawLocalFS to exec a process to get the
> permissions
> ----------------------------------------------------------------------------------------
>
> Key: HADOOP-14600
> URL: https://issues.apache.org/jira/browse/HADOOP-14600
> Project: Hadoop Common
> Issue Type: Bug
> Components: fs
> Affects Versions: 2.7.3
> Environment: file:// in a dir with many files
> Reporter: Steve Loughran
> Assignee: Ping Liu
> Attachments: HADOOP-14600.001.patch, HADOOP-14600.002.patch,
> TestRawLocalFileSystemContract.java
>
>
> Reported in SPARK-21137. a {{FileSystem.listStatus}} call really craws
> against the local FS, because {{FileStatus.getPemissions}} call forces
> {{DeprecatedRawLocalFileStatus}} tp spawn a process to read the real UGI
> values.
> That is: for every other FS, what's a field lookup or even a no-op, on the
> local FS it's a process exec/spawn, with all the costs. This gets expensive
> if you have many files.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
