HADOOP-18807. Close child file systems in ViewFileSystem when cache is disabled. #5847
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhangshuyan0
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jul 17, 2023
There was a problem hiding this comment.
@zhangshuyan0 Thanks for your works. HADOOP-15565 try to fix this issue, does it not solve or involve new one?
| if (!enableInnerCache) { | ||
| for (InodeTree.MountPoint<FileSystem> mountPoint : | ||
| fsState.getMountPoints()) { | ||
| FileSystem targetFs = mountPoint.target.getTargetFileSystemForClose(); |
There was a problem hiding this comment.
how about to invoke getTargetFileSystem directly?
| @@ -413,6 +413,11 @@ public T getTargetFileSystem() throws IOException { | |||
| } | |||
| return targetFileSystem; | |||
| } | |||
|
|
|||
| T getTargetFileSystemForClose() throws IOException { | |||
There was a problem hiding this comment.
what difference between this method and getTargetFileSystem?
There was a problem hiding this comment.
Some target FileSystem may not have be initialized. getTargetFileSystem initializes all target FileSystems (line1039 below). It's costly and unnecessary when we just try to close them.
Hexiaoqiao
changed the title
|
@Hexiaoqiao Thanks for your review. HADOOP-15565 close child filesystems based on the inner cache of ViewFileSystem. If we disable it , namely set |
|
Make sense to me. +1. |
steveloughran
requested changes
Jul 17, 2023
...p-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/ViewFileSystem.java
Show resolved
Hide resolved
| if (child != null) { | ||
| String disableCacheName = String.format("fs.%s.impl.disable.cache", | ||
| child.getUri().getScheme()); | ||
| if (config.getBoolean(disableCacheName, false)) { |
There was a problem hiding this comment.
should we check the child config rather than the viewfs one? would they ever be different?
There was a problem hiding this comment.
Child file system always use their parent's config. We can take a look at line 346 and line 350 of the code below:
There was a problem hiding this comment.
@steveloughran means that we should only check viewfs config, rather than every child config here. As you mentioned code segment, it also shows that check viewfs only is enough IMO.
| */ | ||
| package org.apache.hadoop.fs.viewfs; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
check import ordering
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| public class TestViewFileSystemClose { |
There was a problem hiding this comment.
extend AbstractHadoopTestBase for timeouts, thread naming,...
| FileSystem[] children = viewFs.getChildFileSystems(); | ||
| viewFs.close(); | ||
| FileSystem.closeAll(); | ||
| for (FileSystem fs : children) { |
There was a problem hiding this comment.
use LambdaTestUtils.intercept() here
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.hadoop.fs.viewfs; |
There was a problem hiding this comment.
if this test uses file:// as the childen fs then it could be added in hadoop common, which is where viewfs is implemented. Doing that means that yetus will run the test on every change to hadoop-common, which is needed to stop regressions in viewfs only being found later
There was a problem hiding this comment.
Sorry I don't fully understand what you mean. Do you suggest moving this test file to hadoop-common?
There was a problem hiding this comment.
yes, and to use the localfs. yetus doesnt run the hadoop-hdfs tests when a patch just goes near hadoop-common, so the test needs to get there.
There was a problem hiding this comment.
When the cache is disabled, LocalFileSystem#close() didn't do anything special. That is to say, when the child filesystem is a LocalFileSystem, whether it is closed or not has little effect. So I think using DistributedFileSystem in this UT is more appropriate.
There was a problem hiding this comment.
yes, and to use the localfs. yetus doesnt run the hadoop-hdfs tests when a patch just goes near hadoop-common, so the test needs to get there.
Hi @steveloughran this change can trigger hadoop-hdfs unit test, but it could not ensure to be launched later actually. I am also concerned it could not cover this case if use local FS only here as @zhangshuyan0 mentioned above 'LocalFileSystem#close() didn't do anything special'. Any suggestions? Thanks.
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran Thanks for your review and suggestions, which are very valuable. I have updated this PR based on your comment. Would you mind taking a look again? |
|
💔 -1 overall
This message was automatically generated. |
| @@ -17,26 +17,26 @@ | |||
| */ | |||
| package org.apache.hadoop.fs.viewfs; | |||
|
|
|||
| import static org.apache.hadoop.test.LambdaTestUtils.intercept; | |||
There was a problem hiding this comment.
can you put the static imports right down at the bottom, just above the class declaration?
| String disableCacheName = String.format("fs.%s.impl.disable.cache", | ||
| child.getUri().getScheme()); | ||
| if (config.getBoolean(disableCacheName, false)) { | ||
| child.close(); |
There was a problem hiding this comment.
do failures need to be handled here? if so: what?
any fs set to delete files on close may raise exceptions here from permissions...should they be ignored, fail fast (as this pr does) or should we try best effort to close the others.
The simple patch here is better than today, so i'm not too worried -and that delete on close stuff isn't something to use in production
There was a problem hiding this comment.
Thanks for your advice. I will catch IOException here just like ViewFileSystem.InnerCache#closeAll.
steveloughran
approved these changes
Jul 19, 2023
Hexiaoqiao
approved these changes
Jul 20, 2023
There was a problem hiding this comment.
LGTM. +1.
@steveloughran Please feel free to check in. Thanks.
|
merged to trunk. @zhangshuyan0 can you do a pr for this against branch-3.3 and once it gets past yetus, I will merge that too. thanks |
zhangshuyan0
added a commit
to zhangshuyan0/hadoop
that referenced
this pull request
Jul 20, 2023
…s disabled. (apache#5847) Contributed by Shuyan Zhang
zhangshuyan0
added a commit
to zhangshuyan0/hadoop
that referenced
this pull request
Jul 20, 2023
…s disabled. (apache#5847) Contributed by Shuyan Zhang
|
@steveloughran Done. |
|
got it: #5865 |
steveloughran
pushed a commit
that referenced
this pull request
Jul 21, 2023
…s disabled. (#5847) Contributed by Shuyan Zhang
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…s disabled. (apache#5847) Contributed by Shuyan Zhang
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
When the cache is configured to disabled (namely,
fs.viewfs.enable.inner.cache=falseandfs.*.impl.disable.cache=true), even ifFileSystem.close()is called, the client cannot truly close the child file systems in a ViewFileSystem. This caused our long-running clients to constantly produce resource leaks.How was this patch tested?
Add a new unit test.