hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bryan Duxbury (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-2555) Refactor the HTable#get and HTable#getRow methods to avoid repetition of retry-on-failure logic
Date Sat, 12 Jan 2008 03:15:34 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-2555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12558191#action_12558191
] 

Bryan Duxbury commented on HADOOP-2555:
---------------------------------------

Generally, I love this patch. It'll nicely reduce the amount of copy-paste we have.

I'd like to maybe take it one step further, though. Instead of the existing callServerWithRetries,
how about something like:

{code}

  protected abstract class ServerCallable<T> implements Callable<T> {
    HRegionLocation location;
    HRegionInterface server;
    Text row;
    
    protected ServerCallable(Text row){
      this.row = row;
    }
    
    void instantiateServer(boolean reload) throws IOException {
      if (reload) {
        tableServers = connection.reloadTableServers(tableName);
      }
      location = getRegionLocation(row);
      server = connection.getHRegionConnection(location.getServerAddress());
    }
  }
  
  protected <T> T getRegionServerWithRetries(ServerCallable<T> callable) 
  throws IOException, UnexpectedCallableException {
    for(int tries = 0; tries < numRetries; tries++) {
      try {
        callable.instantiateServer(tries == 0);
        return callable.call();
      } catch (IOException e) {
        if (e instanceof RemoteException) {
          e = RemoteExceptionHandler.decodeRemoteException((RemoteException) e);
        }
        if (tries == numRetries - 1) {
          throw e;
        }
        if (LOG.isDebugEnabled()) {
          LOG.debug("reloading table servers because: " + e.getMessage());
        }
      } catch (Exception e) {
        throw new UnexpectedCallableException(e);
      }
      try {
        Thread.sleep(pause);
      } catch (InterruptedException e) {
        // continue
      }
    }
    return null;    
  }
{code}

which takes us from 

{code}
value = this.callServerWithRetries(new Callable<MapWritable>() {
  public MapWritable call() throws IOException {
    HRegionLocation r = getRegionLocation(row);
    HRegionInterface server =
      connection.getHRegionConnection(r.getServerAddress());
    return server.getRow(r.getRegionInfo().getRegionName(), row, ts);
  }
});
{code}

to 

{code}
value = this.callServerWithRetries(new ServerCallable<MapWritable>(row) {
  public MapWritable call() throws IOException {
  return server.getRow(location.getRegionInfo().getRegionName(), row, ts);
  }
});
{code}

This would save a few lines of code inside each internal block, move a little more logic into
the helper method, and generally jive better with the way my HADOOP-2443 patch is going to
need to work in the near future. 

Comments?

> Refactor the HTable#get and HTable#getRow methods to avoid repetition of retry-on-failure
logic
> -----------------------------------------------------------------------------------------------
>
>                 Key: HADOOP-2555
>                 URL: https://issues.apache.org/jira/browse/HADOOP-2555
>             Project: Hadoop
>          Issue Type: Improvement
>          Components: contrib/hbase
>            Reporter: Peter Dolan
>            Priority: Minor
>         Attachments: hadoop-2555.patch
>
>
> The following code is repeated in every one of HTable#get and HTable#getRow methods:
> {code:title=HTable.java|borderStyle=solid}
>     MapWritable value = null;
>     for (int tries = 0; tries < numRetries; tries++) {
>       HRegionLocation r = getRegionLocation(row);
>       HRegionInterface server =
>         connection.getHRegionConnection(r.getServerAddress());
>       
>       try {
>         value = server.getRow(r.getRegionInfo().getRegionName(), row, ts);  // This is
the only line of code that changes significantly between methods
>         break;
>         
>       } catch (IOException e) {
>         if (e instanceof RemoteException) {
>           e = RemoteExceptionHandler.decodeRemoteException((RemoteException) e);
>         }
>         if (tries == numRetries - 1) {
>           // No more tries
>           throw e;
>         }
>         if (LOG.isDebugEnabled()) {
>           LOG.debug("reloading table servers because: " + e.getMessage());
>         }
>         tableServers = connection.reloadTableServers(tableName);
>       }
>       try {
>         Thread.sleep(this.pause);
>         
>       } catch (InterruptedException x) {
>         // continue
>       }
>     }
> {code}
> This should be factored out into a protected method that handles retry-on-failure logic
to facilitate more robust testing and the development of new API methods.
> Proposed modification:
> // Execute the provided Callable against the server
> protected <T> callServerWithRetries(Callable<T> callable) throws RemoteException;
> The above code could then be reduced to:
> {code:title=HTable.java|borderStyle=solid}
>     MapWritable value = null;
>     final connection;
>     try {
>       value = callServerWithRetries(new Callable<MapWritable>() {
>             HRegionLocation r = getRegionLocation(row);
>             HRegionInterface server =
>                 connection.getHRegionConnection(r.getServerAddress());
>             server.getRow(r.getRegionInfo().getRegionName(), row, ts);
>           });
>     } catch (RemoteException e) {
>       // handle unrecoverable remote exceptions
>     }
> {code}
> This would greatly ease the development of new API methods by reducing the amount of
code needed to implement a new method and reducing the amount of logic that needs to be tested
per method.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message