hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-7430) Refactor the BlockScanner to use O(1) memory and use multiple threads
Date Sat, 20 Dec 2014 00:49:14 GMT

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

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

I rebased on trunk.

I moved the call to {{BlockScanner#addVolumeScanner}} into {{FsVolumeList}}.  Since the call
to {{BlockScanner#removeVolumeScanner}} was there, it seems to make more sense this way.

bq. DFSConfigKeys, I agree the spacing is erratic in this files, but adding some spaces to
line up the variable names would agree with the variables immediately around the new keys

If the spacing is erratic, let's fix the spacing.  I filed HDFS-7557 to do this.  It will
be nicer to do it in a separate jira to avoid muddying the change history.

bq. Still need javadoc <p/> tags in a lot of places. It's not a big deal, so if you
do another pass and think it looks fine we can leave it.

I did another pass on this and found a few spots I missed the first time.  I agree that we
can do a follow-on if there are any more... it's not critical, just kind of nice.

bq. TestFsDatasetImpl, FsVolumeImpl, FsDatasetSpi, FsDatasetImpl unused imports

OK, I just removed a few and all of them show up as black in Intellij.  Hopefully I got them
all this time

bq. \@VisibleForTesting could be added to BlockScanner#Conf#INTERNAL_DFS_BLOCK_SCANNER_THRESHOLD...

added

bq. Still some lines longer than 80 chars

-> look at 

bq. VolumeScanner#positiveMsToHours, the else case \[could be done with TimeUnit\]

Changed.  Also fixed some cases where {{TimeUnit#convert}} was being used improperly by me
(src and dst units were flipped)

bq. testScanRateImpl \[could be done with TimeUnit\]

This is probably a dumb question, but I'm not sure what you are suggesting I should use {{TimeUnit}}
for here?

bq. I'd still like to use JSON to save the iterator  Pretty sure Jackson can pretty print
it for you.

OK.  I'll use JSON here.

bq. I also still like the iterator-of-iterators idea a lot, since we could probably use the
same iterator implementation at each level. Iterating would be simpler, the serde would be
harder, but overall I think simpler code and more friendly for Java programmers.

We'll have time to look into this later.  Rebasing a big patch like this is annoying, and
it's debatable whether iterator-of-iterators is even better (it's certainly less efficient)

bq. BlockIterator still implements Closeable, unnecessary?

It might be necessary for other implementations of BlockIterator, besides the one in {{FsDatasetImpl}}.
 In general, you might need to create some storage-system-level iterator.

bq. \[neededBytesPerSec\] Still mismatched?

Very good catch.  Yeah, it needs to convert between bytes/hour and bytes/sec

bq. Guessing the JDK7 file listing goodness is coming in the next patch, since it's still
using File#list

Now that I rebased on trunk, I can add this.  Added.

bq. Did you look into the failed test I posted earlier? Any RCA?

I think that this probably resulted from the incorrect rate computation.

bq. The bugs found in my previous review seem worth unit testing, e.g the OBO with the binarySearch
index and the neededBytesPerSec that still looks off, the <= in place of < that affected
continuous scans. Might be fun trying to write some actual stripped down unit tests, rather
than poking with a full minicluster.

OK that's fair.  The binarySearch index code path didn't get much exercise, because the off-by-one
appeared only when the directory listing had changed between our last listDir and the current
one.  I wrote a unit test just for {{FsVolumeImpl#nextSorted}} that should verify that this
is working as expected.

I will also break {{calculateNeededBytesPerSec}} out into a function and write a unit test
for that

> Refactor the BlockScanner to use O(1) memory and use multiple threads
> ---------------------------------------------------------------------
>
>                 Key: HDFS-7430
>                 URL: https://issues.apache.org/jira/browse/HDFS-7430
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>    Affects Versions: 2.7.0
>            Reporter: Colin Patrick McCabe
>            Assignee: Colin Patrick McCabe
>         Attachments: HDFS-7430.002.patch, HDFS-7430.003.patch, HDFS-7430.004.patch, HDFS-7430.005.patch,
HDFS-7430.006.patch, memory.png
>
>
> We should update the BlockScanner to use a constant amount of memory by keeping track
of what block was scanned last, rather than by tracking the scan status of all blocks in memory.
 Also, instead of having just one thread, we should have a verification thread per hard disk
(or other volume), scanning at a configurable rate of bytes per second.



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

Mime
View raw message