-
Couldn't load subscription status.
- Fork 9.1k
HADOOP-17028. ViewFS should initialize mounted target filesystems lazily #2260
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
base: trunk
Are you sure you want to change the base?
Conversation
ebe4957 to
0d01d8d
Compare
|
💔 -1 overall
This message was automatically generated. |
0d01d8d to
fc33848
Compare
|
@umamaheswararao Can you please review this PR when you get a chance |
|
💔 -1 overall
This message was automatically generated. |
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.
I like the lazy eval style
- thread safety: what is the story here?
- what's the story w.r.t IOExceptions being raised in the init function?
in org.apache.hadoop.fs.impl.FunctionsRaisingIOE we have an interface for functions which explicilty raise them. Using that or something like it makes sense here
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/viewfs/InodeTree.java
Outdated
Show resolved
Hide resolved
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.
make it a PathIOException with the given path.
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.
move to java import group
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.
import placement
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.
import placement
fc33848 to
e6fefc0
Compare
|
💔 -1 overall
This message was automatically generated. |
Thanks @steveloughran for the review. |
e6fefc0 to
c772403
Compare
|
💔 -1 overall
This message was automatically generated. |
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.
Someone else needs to review the core functionality, but I like the changes. Still need to get that sync block right though. FWIW, long init has caused problems with Hive in the past, hence work in s3a to try and reduce the amount of network IO here. Lazy init too hard work to justify though
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.
nit: add a . to keep javadoc happy
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.
added . at the end of the line
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.
nit, curly { }
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.
Done
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.
this will dual init as thread #2 will block. You need another check inside the sync block so the second thread won't repeat itself
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.
Added extra check
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.
log the full stack trace
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.
Changed the code.
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.
FYI. I've got a WrappedIOException, but in #2069 making it a public org.apache.hadoop.fs.functional.RuntimeIOException whose cause is only ever IOE. Not something to be picked up here (yet), but worth knowing. I'm trying to make the functional & lambda/expression stuff more useable,
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.
+full exception log
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.
Changed the code.
|
Thanks @steveloughran for review! I am wondering this call will make all fs to be initialized. That means we are not getting this lazy initialization benefit fully. |
c772403 to
9319754
Compare
|
@umamaheswararao @steveloughran bumping up the conversation on this thread. Recently we have faced lot of issues (OOM, fs initialization time going up) because of not having lazy initialization. |
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.
It looks like @steveloughran deprecated this class later after his comment. Don't know what prompted the refactoring, but
import org.apache.hadoop.fs.impl.FunctionsRaisingIOE.FunctionRaisingIOE
should be now
org.apache.hadoop.util.functional.FunctionRaisingIOE
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.
replaced org.apache.hadoop.fs.impl.FunctionsRaisingIOE.FunctionRaisingIOE with org.apache.hadoop.util.functional.FunctionRaisingIOE
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.
You can call closeAll() here, which clears the cache completely. And the you can compare with 0 in the assert.
73a5661 to
c66c2bf
Compare
|
💔 -1 overall
This message was automatically generated. |
142d2ff to
7708fdd
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
7708fdd to
d8aac16
Compare
|
💔 -1 overall
This message was automatically generated. |
d8aac16 to
2f8e9a6
Compare
|
💔 -1 overall
This message was automatically generated. |
2f8e9a6 to
8fa62c8
Compare
|
💔 -1 overall
This message was automatically generated. |
8fa62c8 to
a658312
Compare
|
💔 -1 overall
This message was automatically generated. |
|
Looks good generally. I checked the tests are catching previous errors.
|
a658312 to
541e432
Compare
@shvachko I have removed the checkstyle warning. Also added the individual imports and removed the empty line |
|
🎊 +1 overall
This message was automatically generated. |
|
+1 on the latest patch. |
|
💔 -1 overall
This message was automatically generated. |
…ily. Contributed by Abhishek Das (#2260)
|
💔 -1 overall
This message was automatically generated. |
…ily. Contributed by Abhishek Das (apache#2260) (cherry picked from commit 1dd03cc)
…ily. Contributed by Abhishek Das (apache#2260)
https://issues.apache.org/jira/browse/HADOOP-17028