[
https://issues.apache.org/jira/browse/HADOOP-15565?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16922760#comment-16922760
]
Erik Krogen commented on HADOOP-15565:
--------------------------------------
Hi [~LiJinglun], sorry for my delayed response, I just returned from vacation
recently. Here is another review. Things look great overall, the comments are
mostly minor.
* Can we make the log message in {{ViewFileSystem}} L119 more informative? If
you were only to look at the logs and not the code, "close failed" would be
confusing.
* Can you add some comments on {{ViewFileSystem}} L292 explaining why the cache
can be immutable, on {{InnerCache}} explaining why this cache is necessary
(maybe just a reference to this JIRA), and on {{InnerCache.Key}} describing why
it is okay to use a simple key here (as we discussed previously, no need for
UGI)?
* The tests in {{TestViewFileSystemHdfs}} LGTM, but I don't think they are
HDFS-specific. Can we put them in {{ViewFileSystemBaseTest}}? Also you have one
typo, {{testViewFilsSystemInnerCache}} should be
{{testViewFileSystemInnerCache}}
* Can you describe why the changes in {{TestViewFsDefaultValue}} are necessary?
* Can you explain why the changes in
{{TestViewFileSystemDelegationTokenSupport}} are necessary? Same for
{{TestViewFileSystemDelegation}} -- it seems like the old way of returning the
created {{fs}} was cleaner? I also don't understand the need for changes in
{{testSanity()}} -- does the string comparison no longer work?
> ViewFileSystem.close doesn't close child filesystems and causes FileSystem
> objects leak.
> ----------------------------------------------------------------------------------------
>
> Key: HADOOP-15565
> URL: https://issues.apache.org/jira/browse/HADOOP-15565
> Project: Hadoop Common
> Issue Type: Bug
> Reporter: Jinglun
> Assignee: Jinglun
> Priority: Major
> Attachments: HADOOP-15565.0001.patch, HADOOP-15565.0002.patch,
> HADOOP-15565.0003.patch, HADOOP-15565.0004.patch, HADOOP-15565.0005.patch,
> HADOOP-15565.0006.patch
>
>
> ViewFileSystem.close() does nothing but remove itself from FileSystem.CACHE.
> It's children filesystems are cached in FileSystem.CACHE and shared by all
> the ViewFileSystem instances. We could't simply close all the children
> filesystems because it will break the semantic of FileSystem.newInstance().
> We might add an inner cache to ViewFileSystem, let it cache all the children
> filesystems. The children filesystems are not shared any more. When
> ViewFileSystem is closed we close all the children filesystems in the inner
> cache. The ViewFileSystem is still cached by FileSystem.CACHE so there won't
> be too many FileSystem instances.
> The FileSystem.CACHE caches the ViewFileSysem instance and the other
> instances(the children filesystems) are cached in the inner cache.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
