Joe McDonnell has posted comments on this change. ( )

Change subject: IMPALA-6638: Reduce file handle cache lock contention

Patch Set 2:

File be/src/runtime/io/handle-cache.inline.h:
PS2, Line 128: This only happens if there is no entry for this file or all the
             :   // existing file handles are in use.
> That was true while holding the lock above but is no longer necessarily tru
Changed this to insert in the front and updated this comment about why. Also 
added a comment when iterating above to explain why iterating in order is 
PS2, Line 130:  matter whether we insert this entry before or after or in 
between the
             :   // existing entries for this file
> why is the previous sentence needed in order for this to be okay?
This was poorly worded. What I meant is that not much time has passed since we 
saw that there wasn't any entry available. Any ordering decision is mostly 
unimportant under that circumstance.

I thought about it some more, and we are very slightly better off explicitly 
specifying the ordering. It is mostly an edge case, but inserting at the front 
solves it.
PS2, Line 134:   FileHandleEntry entry(new_fh, p.lru_list);
             :   typename MapType::iterator new_it = p.cache.emplace(*fname, 
> I would remove the constructor and move the arguments to emplace: emplace(*
I appreciate the comment, but I try to avoid style changes in adjacent code.
PS2, Line 137:   ++p.size;
             :   if (p.size > p.capacity) EvictHandles(p);
> I would prefer to move this before the creation of the new entry, to make i
I appreciate the comment, but I try to avoid style changes in adjacent code.
PS2, Line 139:   DCHECK(!new_elem->in_use);
> This DCHECK could be probably removed.
This could go either way. We are relying on the constructor setting in_use to 
false. I figure a DCHECK doesn't hurt.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c695b21ca556e9c73c703c0c891e64939271c8d
Gerrit-Change-Number: 9576
Gerrit-PatchSet: 2
Gerrit-Owner: Joe McDonnell <>
Gerrit-Reviewer: Csaba Ringhofer <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Joe McDonnell <>
Gerrit-Comment-Date: Mon, 12 Mar 2018 21:51:19 +0000
Gerrit-HasComments: Yes

Reply via email to