[
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.
