jbrennan333 commented on a change in pull request #2511:
URL: https://github.com/apache/hadoop/pull/2511#discussion_r542744744
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
##########
@@ -92,21 +91,15 @@
private long lastHolderUpdateTime;
private String internalLeaseHolder;
+ //
// Used for handling lock-leases
// Mapping: leaseHolder -> Lease
- private final SortedMap<String, Lease> leases = new TreeMap<>();
- // Set of: Lease
- private final NavigableSet<Lease> sortedLeases = new TreeSet<>(
- new Comparator<Lease>() {
- @Override
- public int compare(Lease o1, Lease o2) {
- if (o1.getLastUpdate() != o2.getLastUpdate()) {
- return Long.signum(o1.getLastUpdate() - o2.getLastUpdate());
- } else {
- return o1.holder.compareTo(o2.holder);
- }
- }
- });
+ // TreeMap has O(log(n)) complexity but it is more space efficient
+ // compared to HashMap. Therefore, replacing TreeMap with a
+ // HashMap can be considered to get faster O(1) time complexity
+ // on the expense of 30% memory waste.
+ //
Review comment:
This explanation belongs in the Jira, not in a comment in the code.
When looking at current code, it's not really clear why you are talking about
TreeMap at all.
Also, I think this comment misses the point of the TreeMap. The reason a
TreeMap was used here was to maintain a sorted order, which allowed the
checkLeases() to exit the while loop as soon as it hit an unexpired lease.
The new design removes the need for the TreeMap by pruning the list it
passes to checkLeases().
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
##########
@@ -541,10 +550,10 @@ public void run() {
fsnamesystem.getEditLog().logSync();
}
}
-
- Thread.sleep(fsnamesystem.getLeaseRecheckIntervalMs());
} catch(InterruptedException ie) {
- LOG.debug("{} is interrupted", name, ie);
+ if (LOG.isDebugEnabled()) {
Review comment:
This LOG.isDebugEnabled() check is not needed anymore.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
##########
@@ -515,6 +508,13 @@ public void setLeasePeriod(long softLimit, long hardLimit)
{
this.softLimit = softLimit;
this.hardLimit = hardLimit;
}
+
+ private synchronized Collection<Lease> getExpiredCandidateLeases() {
+ final long now = Time.monotonicNow();
+ return leases.values().stream()
+ .filter(lease -> lease.expiredHardLimit(now))
+ .collect(Collectors.toCollection(HashSet::new));
+ }
Review comment:
I much prefer the loop in @daryn-sharp's original code.
Collection<Lease> expired = new HashSet<>();
for (Lease lease : leases) {
if (lease.expiredHardLimit(now)) {
expired.add(lease);
}
}
This streams code will have to change if we want to pull this back to branch
2.
I think @daryn-sharp also said that stream()'s are more expensive.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
