[ 
https://issues.apache.org/jira/browse/HDFS-8873?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14804846#comment-14804846
 ] 

Colin Patrick McCabe commented on HDFS-8873:
--------------------------------------------

Thanks, [~templedf].  Looking better.

The configuration looks good.  We had a quick offline discussion where 
[~templedf] pointed out that we could add a selectable throttle type in the 
future in a compatible way.  So we don't need to have the throttle type 
selector in this patch.

{code}
46        public DirectoryScannerThrottle(int limit) {
47          if ((limit <= 0) || (limit >= 1000)) {
{code}
We use "1000" a lot in this code.  Can we have a constant like {{MS_PER_SEC}} 
to make its role clearer?

{code}
109       public synchronized void cycle() throws InterruptedException {
110         while (!open) {
111           try {
112             wait();
113           } catch (InterruptedException ex) {
114             // Ignore
115           }
116         }
{code}
Why are we declaring that the function throws {{InterruptedException}} if we 
just catch it and do nothing?

In {{ReportCompiler#run}}, we should probably at least call 
{{Thread.currentThread.interrupt}} rather than swallowing the 
{{InterruptedException}} completely.  As described in 
http://www.ibm.com/developerworks/library/j-jtp05236/ , "even noncancelable 
tasks should attempt to preserve the interrupted status in case code higher up 
on the call stack wants to act on the interruption after the noncancelable task 
completes."

{code}
84        @VisibleForTesting
85        final Map<String, Stats> stats = new HashMap<>();
{code}
It's not clear from looking at this what the String key is all about.  Either 
document that it is a block pool id in the comment, or rename the variable to 
{{statsPerBpId}} or similar.

{code}
508               int d = 0; // index for blockpoolReport
509               int m = 0; // index for memReprot
{code}
Rename d to blockReportIdx, m to memReportIdx?  I don't normally complain about 
short variable names, but this loop is somewhat complex.

{code}
683         // No need to set an initial capacity because the number of entries 
will
684         // never be that large
{code}
All very true, but you could have just initialized it with reports.size() and 
saved some typing :)

ThrottleTask / UnthrottleTask: is there any way we could reuse these objects 
and avoid generating all that garbage?  It seems like they are stateless 
(although they alter state inside the DirectoryScannerThrottle).  We could 
probably just keep one of each inside the parent class and avoid allocating.

{code}
      long mark = Time.monotonicNow();
{code}
Please make this markMs.  Same for newMark.

{code}
913               // Skip all the files that cycle with block name until
914               // getting to the metafile for the block
915               while ((i + 1 < files.length) && files[i + 1].isFile()
916                       && files[i + 
1].getName().startsWith(blockFile.getName())) {
917                 i++;
918     
919                 if (isBlockMetaFile(blockFile.getName(), 
files[i].getName())) {
920                   metaFile = files[i];
921                   break;
922                 }
923               }
{code}
I realize you didn't change this in your patch, but this is a potentially very 
expensive operation depending on the directory size.  Perhaps we should sort 
the array of children at the lowest level, so that we know that the meta file 
is the next entry, if it exists.  We might want to do that in a follow-on jira.

> throttle directoryScanner
> -------------------------
>
>                 Key: HDFS-8873
>                 URL: https://issues.apache.org/jira/browse/HDFS-8873
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>          Components: datanode
>    Affects Versions: 2.7.1
>            Reporter: Nathan Roberts
>            Assignee: Daniel Templeton
>         Attachments: HDFS-8873.001.patch, HDFS-8873.002.patch, 
> HDFS-8873.003.patch
>
>
> The new 2-level directory layout can make directory scans expensive in terms 
> of disk seeks (see HDFS-8791) for details. 
> It would be good if the directoryScanner() had a configurable duty cycle that 
> would reduce its impact on disk performance (much like the approach in 
> HDFS-8617). 
> Without such a throttle, disks can go 100% busy for many minutes at a time 
> (assuming the common case of all inodes in cache but no directory blocks 
> cached, 64K seeks are required for full directory listing which translates to 
> 655 seconds) 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to