[jira] [Comment Edited] (HDFS-7430) Refactor the BlockScanner to use O(1) memory and use multiple threads

["Colin Patrick McCabe (JIRA)" <ji...@apache.org>]

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

Colin Patrick McCabe edited comment on HDFS-7430 at 12/20/14 2:50 AM:
----------------------------------------------------------------------

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

I think I've gotten rid of all these, except for in import statements and configuration keys.  There are many in the deleted code, though :)

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


was (Author: cmccabe):
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, HDFS-7430.007.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)