Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/1578#discussion_r15430260
  
    --- Diff: 
core/src/main/scala/org/apache/spark/storage/BlockFetcherIterator.scala ---
    @@ -200,14 +200,21 @@ object BlockFetcherIterator {
           // these all at once because they will just memory-map some files, 
so they won't consume
           // any memory that might exceed our maxBytesInFlight
           for (id <- localBlocksToFetch) {
    -        getLocalFromDisk(id, serializer) match {
    -          case Some(iter) => {
    -            // Pass 0 as size since it's not in flight
    -            results.put(new FetchResult(id, 0, () => iter))
    -            logDebug("Got local block " + id)
    +        try{
    +          getLocalFromDisk(id, serializer) match {
    +            case Some(iter) => {
    +              // Pass 0 as size since it's not in flight
    +              results.put(new FetchResult(id, 0, () => iter))
    +              logDebug("Got local block " + id)
    +            }
    +            case None => {
    +              throw new BlockException(id, "Could not get block " + id + " 
from local machine")
    +            }
               }
    -          case None => {
    -            throw new BlockException(id, "Could not get block " + id + " 
from local machine")
    +        } catch {
    +          case e: Exception => {
    +            logError(s"Error occurred while fetch local block $id", e)
    +            results.put(new FetchResult(id, -1, null))
               }
    --- End diff --
    
    Actually, getLocalFromDisk never return None but can throw BlockException. 
so I think "case None" block above is useless and we should remove the "case 
None" block rather than doing results.put.
    
    > Is there any other kind of error that can happen beyond getLocalFromDisk 
returning None?
    Yes, BlockException is thrown from getLocalFromDisk, and 
FileNotFoundException from DiskStore#getBytes when it failed to fetch 
shuffle_*_* from local disk. 
    
    > Also, the current code seems to forget the exception: it just puts in a 
failed result. Is this intentional, i.e. will get a FetchFailedException later?
    It's for get FetchFailedException later. If we return from 
BasicBlockFetchIterator#getLocallocks, we can't know whether rest of blocks can 
be read successfully or not.
    
    
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to