HDFS-15987. Improve oiv tool to parse fsimage file in parallel with d… #2918
Conversation
This file contains hidden or 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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Apr 17, 2021
| * | ||
| * STEP2: Merge files. | ||
| */ | ||
| private void outputInParallel(Configuration conf, FileSummary summary, |
There was a problem hiding this comment.
It is a great idea to improve OIV performance, but it seems only considering INODE_SUB section now, right? Actually there are also some other subsections, such as INODE_REFERENCE_SUB, INODE_DIR_SUB, SNAPSHOT_DIFF_SUB. I think we could process them using the same way.
cc @sodonnel @jojochuang any ideas for this improvement?
There was a problem hiding this comment.
Thanks @Hexiaoqiao for guidance. Other possible sub-sections can also be optimized, but may not be the focus of optimization, i think. Analyse as below.
There are several steps to parse fsimage in the case of DELIMITED format:
-
- Load string table
-
- Load inode references
-
- Handle INODE to memory or levelDB
-
- Handle INODE_DIR to memory or levelDB
-
- Output INODE
For example In our practice, it takes 7 hours for to parse a large fsimage file, and just the 5th step which only uses INODE takes more than 6 hours. So I did parallelization in 5th step.
The 3rd and 4th steps are basically memory operations, which are not very time-consuming. It may be possible to use INODE_SUB or INODE_DIR_SUB feature for parallel processing, but I am not sure if it is necessary to do so.
Hope to discuss further to clarify whether other sub-sections need to be processed, Thanks!
There was a problem hiding this comment.
I think its a good idea to make this change in stages. If using just the parallel part of the INODE sections cuts the runtime in half, then its a good change. We can always open more Jiras to parallel the other sections after this one is done, if it makes sense.
There was a problem hiding this comment.
Make sense to me. Leave some nit comment inline.
Hexiaoqiao
reviewed
Apr 17, 2021
sync trunk
All failed tests are passed in local. It seems unrelated to this PR. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Hexiaoqiao
reviewed
Jun 1, 2021
| @@ -132,6 +133,7 @@ private static Options buildOptions() { | |||
| options.addOption("delimiter", true, ""); | |||
| options.addOption("sp", false, ""); | |||
| options.addOption("t", "temp", true, ""); | |||
| options.addOption("threads", true, ""); | |||
There was a problem hiding this comment.
I am concerned that parameter -threads will collide with -t, and it could be parsed to -t hreads here. It is safe to changes names to another one to avoid abuse.
There was a problem hiding this comment.
Yes, it should be renamed and will be fixed in next commit. By the way, is there a suitable parameter name recommendation ? 😊
There was a problem hiding this comment.
This option name threads has to rename here. Otherwise it will conflict with -t.
There was a problem hiding this comment.
This option name
threadshas to rename here. Otherwise it will conflict with-t.
New option as follow. Point it out If any better option, thanks!
options.addOption("m", "multiThread", true, "");| @@ -146,7 +146,13 @@ public String build() { | |||
| PBImageDelimitedTextWriter(PrintStream out, String delimiter, | |||
| String tempPath, boolean printStoragePolicy) | |||
| throws IOException { | |||
| super(out, delimiter, tempPath); | |||
| this(out, delimiter, tempPath, printStoragePolicy, 1, "-"); | |||
There was a problem hiding this comment.
I am confused why we use static string "-" rather than null. any other consideration?
There was a problem hiding this comment.
Considering the default value of outputFile in OfflineImageViewerPB.java is "-", I continue this style.
String outputFile = cmd.getOptionValue("o", "-");| @@ -640,6 +657,19 @@ long getParentId(long id) throws IOException { | |||
| private void output(Configuration conf, FileSummary summary, | |||
| FileInputStream fin, ArrayList<FileSummary.Section> sections) | |||
| throws IOException { | |||
| ArrayList<FileSummary.Section> allINodeSubSections = | |||
| getINodeSubSections(sections); | |||
| if (numThreads > 1 && !parallelOut.equals("-") && | |||
There was a problem hiding this comment.
the same as above comments.
| * | ||
| * STEP2: Merge files. | ||
| */ | ||
| private void outputInParallel(Configuration conf, FileSummary summary, |
There was a problem hiding this comment.
Make sense to me. Leave some nit comment inline.
|
Add a report Improve_oiv_tool_001.pdf. |
|
🎊 +1 overall
This message was automatically generated. |
|
@whbing This feature is useful for parse fsimage. It is near to check in when fix the comment. Any plan to push this forward? Thanks. |
ferhui
reviewed
Sep 19, 2021
| public void visit(RandomAccessFile file) throws IOException { | ||
| public void visit(String filePath) throws IOException { | ||
| filename = new File(filePath); | ||
| RandomAccessFile file = new RandomAccessFile(filePath, "r"); |
There was a problem hiding this comment.
Maybe it is unnecessary to change here.
Thanks for attention. The PR works well and I will improve some details if needed. |
whbing
commented
Sep 27, 2021
| String compressionCodec, Configuration conf) | ||
| throws IOException { | ||
| // channel of RandomAccessFile is not thread safe, use File | ||
| FileInputStream fin = new FileInputStream(filename); |
There was a problem hiding this comment.
RandomAccessFile is not thread safe.
The change here is to get stream for each thread through File filename
There was a problem hiding this comment.
Maybe it is unnecessary to change here.
@ferhui Thanks for the detailed review.
Considering the thread-safe channel, RandomAccessFile is avoided here. I haven't found a more elegant way for the time being.
|
💔 -1 overall
This message was automatically generated. |
symious
reviewed
Dec 2, 2021
| ArrayList<FileSummary.Section> allINodeSubSections = | ||
| getINodeSubSections(sections); | ||
| if (numThreads > 1 && !parallelOut.equals("-") && | ||
| allINodeSubSections.size() > 1) { |
There was a problem hiding this comment.
It would be good to add some logs showing it's using serial or parallel output. And if the user is using the tool with option "threads" but her fsimage doesn't support the feature, better add some logs notifying her it's falling back to the serial output.
There was a problem hiding this comment.
Good advice. It will be optimized in the next commit. Thanks for review! @sodonnel
| } | ||
| } | ||
|
|
||
| if (exceptions.size() != 0) { |
There was a problem hiding this comment.
Is this check happening after all threads are finished? Can we return the exception promptly?
| mark = end; | ||
| String path = paths[i]; | ||
|
|
||
| threads[i] = new Thread(() -> { |
There was a problem hiding this comment.
Maybe thread pool is better here?
There was a problem hiding this comment.
Maybe thread pool is better here?
@symious Thanks! I will try this suggestion in next commit.
|
It seems that some comments are not fixed now. Any plan to push this improvement forwards? This PR has pending here for near year, we should check in ASAP once ready. Thanks. |
Sorry for the late follow-up and reply. The new commit will be tested later in our cluster and it will be submitted these days. |
|
💔 -1 overall
This message was automatically generated. |
|
The latest commit works well in prod env. @Hexiaoqiao @symious PTAL. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @whbing for your updates. It almost look good to me. Please fix checkstyle issue also. |
|
🎊 +1 overall
This message was automatically generated. |
Hexiaoqiao
approved these changes
Mar 18, 2022
asfgit
pushed a commit
that referenced
this pull request
Mar 22, 2022
…elimited format. (#2918). Contributed by Hongbing Wang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
|
Committed to trunk. Thanks @whbing for your contributions! Thanks all to reviews! |
asfgit
pushed a commit
that referenced
this pull request
Mar 23, 2022
…elimited format. (#2918). Contributed by Hongbing Wang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
|
revert and re-check in to correct the git message. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…elimited format. (apache#2918). Contributed by Hongbing Wang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…l with delimited format. (apache#2918). Contributed by Hongbing Wang." This reverts commit 8897549.
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…elimited format. (apache#2918). Contributed by Hongbing Wang. Signed-off-by: He Xiaoqiao <hexiaoqiao@apache.org>
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.
…elimited format
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute