[
https://issues.apache.org/jira/browse/HDFS-15493?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17173653#comment-17173653
]
Stephen O'Donnell commented on HDFS-15493:
------------------------------------------
Hi [~smarthan] Thanks for the latest patch, and sorry for the slow review. I
missed the email telling me you had uploaded the latest patch.
Just a few minor points to address and one more difficult one:
1. There is a checkstyle warning on line 34 in FSImageFormatPBINode.java for an
unused import.
2. In waitExecutorTerminated, I think the line `long start =
System.currentTimeMillis();` should be above the while loop so start is not
reset on each iteration?
{code}
private void waitExecutorTerminated(ExecutorService executorService)
throws IOException {
executorService.shutdown();
while (!executorService.isTerminated()) {
long start = System.currentTimeMillis();
try {
executorService.awaitTermination(1, TimeUnit.SECONDS);
if (LOG.isDebugEnabled()) {
LOG.debug("Waiting to executor service terminated duration {}ms.",
(System.currentTimeMillis() - start));
}
} catch (InterruptedException e) {
LOG.error("Interrupted waiting for executor terminated.", e);
throw new IOException(e);
}
{code}
3. Our change is loading the inode cache and block map for all inodes in the
image when loading the inode section. However, when we are loading the
inodeDirectory section, we are still calling `fillUpInodeList(...)` for the
`RefChildrenList`. Looking at the code, an inodeReference always refers to
another inode, and it is that referred inode which is added to the cache and
blocks map.
Before this change, there may have been some inodes where were only referred to
in references, but those INodes must be in the inode section of the image.
Therefore we will handle all inodes mentioned in the references already with
the change in `loadINodesInSection(...)` and hence should remove the
`fillUpInodeList(...)` call below - do you think that is correct?
Note that I don't think these extra calls will do any harm, but they will
probably result in doing more work on an image with lots of references (ie
snapshots).
{code}
for (int refId : e.getRefChildrenList()) {
INodeReference ref = refList.get(refId);
if (addToParent(p, ref)) {
fillUpInodeList(inodeList, ref);
} else {
LOG.warn("Failed to add the inode reference {} to the directory {}",
ref.getId(), p.getId());
}
}
{code}
4. Please add a short JavaDoc to `addToCacheInternal(...)` and
`updateBlockMapInternal(...)` indicating they can only be run by a single
thread as they modify non-thread safe data structures. This will help ensure
someone does not make a mistake using them in the future.
5. Maybe add a comment above the two Executor definitions, eg: "These executors
must be single threaded, as they are used to modify structures which are not
thread safe", again to warn someone in the future not to change this:
{code}
blocksMapUpdateExecutor = Executors.newSingleThreadExecutor();
nameCacheUpdateExecutor = Executors.newSingleThreadExecutor();
{code}
After addressing these few points, I think this change will be good to commit.
> Update block map and name cache in parallel while loading fsimage.
> ------------------------------------------------------------------
>
> Key: HDFS-15493
> URL: https://issues.apache.org/jira/browse/HDFS-15493
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: namenode
> Reporter: Chengwei Wang
> Priority: Major
> Attachments: HDFS-15493.001.patch, HDFS-15493.002.patch,
> HDFS-15493.003.patch, HDFS-15493.004.patch, HDFS-15493.005.patch,
> fsimage-loading.log
>
>
> While loading INodeDirectorySection of fsimage, it will update name cache and
> block map after added inode file to inode directory. It would reduce time
> cost of fsimage loading to enable these steps run in parallel.
> In our test case, with patch HDFS-13694 and HDFS-14617, the time cost to load
> fsimage (220M files & 240M blocks) is 470s, with this patch , the time cost
> reduc to 410s.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
