[
https://issues.apache.org/jira/browse/HDFS-6757?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14531562#comment-14531562
]
Jing Zhao commented on HDFS-6757:
---------------------------------
Thanks for continuing the work here, Haohui. Some comments on the latest 013
patch:
# Any reason to use {{o1.lastUpdate}} but use {{o2.getLastUpdate()}} in the
follwoing code?
{code}
+ private final PriorityQueue<Lease> sortedLeases = new
PriorityQueue<Lease>(512,
+ new Comparator<Lease>() {
+ @Override
+ public int compare(Lease o1, Lease o2) {
+ return Long.signum(o1.lastUpdate - o2.getLastUpdate());
+ }
+ });
{code}
# Let's keep this Precondition check in {{getNumUnderConstructionBlocks}}
considering this patch is a big change.
{code}
- Preconditions.checkState(cons.isUnderConstruction());
{code}
# Similarly let's put this check into {{serializeFilesUCSection}} or print a
warning msg if the inode is not UC.
{code}
Preconditions.checkState(node.isUnderConstruction());
{code}
# What is the difference between {{removeLeases}} and {{removeLeaseById}}?
# We'd better add a check to make sure the iip and p are both valid,
i.e., starting from root, to prevent a wrong path written
into the CloseOp editlog.
{code}
INodesInPath iip = INodesInPath.fromINode(fsd.getInode(id));
p = iip.getPath();
boolean completed = fsnamesystem.internalReleaseLease(
leaseToCheck, p, iip,
HdfsServerConstants.NAMENODE_LEASE_HOLDER);
{code}
# Any reason to adding this IOException for loading editlog?
{code}
if (file.isUnderConstruction()) {
if (fsNamesys.leaseManager.getLease(file) == null) {
throw new IOException("UC file " + path + " does not have a " +
"corresponding lease");
}
{code}
# One question is, after we record INode id in lease manager, do we still want
to record full paths of UC files in FSImage?
# In {{INodeFile#destroyAndCollectBlocks}}, which may also be called when
deleting a snapshot, {{removedUCFiles}} may be null? If yes, we need a unit
test to catch the case.
{code}
FileUnderConstructionFeature uc = getFileUnderConstructionFeature();
if (uc != null) {
removedUCFiles.add(getId());
}
{code}
# When {{INodeFile#cleanSubtree}} is called because the file or its ancestor is
deleted,
even if the file is in snapshot, we should check its UC state and update
removedUCFiles accordingly.
# The changes in FileDiff is not needed.
# Maybe we can add extra tests for the delete and rename operations when UC
files are involved. Specifically, a test on the internal lease recovery for
renamed UC files may be useful.
# Nit: the following code needs reformat:
{code}
+ private final HashMap<Long, Lease> leasesById = new
+ HashMap<>();
{code}
and
{code}
LOG.debug("Lease recovery for inode " + id + " is complete. " +
"File" +
" closed.");
{code}
and
{code}
- *
- * @param bsps
+ * @param bsps
{code}
> Simplify lease manager with INodeID
> -----------------------------------
>
> Key: HDFS-6757
> URL: https://issues.apache.org/jira/browse/HDFS-6757
> Project: Hadoop HDFS
> Issue Type: Improvement
> Reporter: Haohui Mai
> Assignee: Haohui Mai
> Labels: BB2015-05-TBR
> Attachments: HDFS-6757.000.patch, HDFS-6757.001.patch,
> HDFS-6757.002.patch, HDFS-6757.003.patch, HDFS-6757.004.patch,
> HDFS-6757.005.patch, HDFS-6757.006.patch, HDFS-6757.007.patch,
> HDFS-6757.008.patch, HDFS-6757.009.patch, HDFS-6757.010.patch,
> HDFS-6757.011.patch, HDFS-6757.012.patch, HDFS-6757.013.patch
>
>
> Currently the lease manager records leases based on path instead of inode
> ids. Therefore, the lease manager needs to carefully keep track of the path
> of active leases during renames and deletes. This can be a non-trivial task.
> This jira proposes to simplify the logic by tracking leases using inodeids
> instead of paths.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
