Skip to content

HBASE-22699 refactor isMetaClearingException #578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 16, 2019
Merged

HBASE-22699 refactor isMetaClearingException #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocatorHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import static org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil.findException;
import static org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil.isMetaClearingException;
import static org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil.isRegionServerOverloadedException;

import java.util.Arrays;
import java.util.Optional;
Expand Down Expand Up @@ -67,7 +68,8 @@ static void updateCachedLocationOnError(HRegionLocation loc, Throwable exception
LOG.debug("The actual exception when updating {} is {}", loc,
cause != null ? cause.toString() : "none");
}
if (cause == null || !isMetaClearingException(cause)) {
if (cause == null || !isMetaClearingException(cause)
|| isRegionServerOverloadedException(cause)) {
LOG.debug("Will not update {} because the exception is null or not the one we care about",
loc);
return;
Expand Down
6 changes: 4 additions & 2 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRequestFutureImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,8 @@ private void receiveGlobalFailure(
// Do not use the exception for updating cache because it might be coming from
// any of the regions in the MultiAction.
updateCachedLocations(server, regionName, row,
ClientExceptionsUtil.isMetaClearingException(t) ? null : t);
ClientExceptionsUtil.isMetaClearingException(t) &&
!ClientExceptionsUtil.isRegionServerOverloadedException(t) ? null : t);
for (Action action : e.getValue()) {
Retry retry = manageError(
action.getOriginalIndex(), action.getAction(), canRetry, t, server);
Expand Down Expand Up @@ -913,7 +914,8 @@ private void invokeCallBack(byte[] regionName, byte[] row, CResult result) {
}

private void cleanServerCache(ServerName server, Throwable regionException) {
if (ClientExceptionsUtil.isMetaClearingException(regionException)) {
if (ClientExceptionsUtil.isMetaClearingException(regionException)
&& !ClientExceptionsUtil.isRegionServerOverloadedException(regionException)) {
// We want to make sure to clear the cache in case there were location-related exceptions.
// We don't to clear the cache for every possible exception that comes through, however.
asyncProcess.connection.clearCaches(server);
Expand Down
3 changes: 2 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionImplementation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1918,7 +1918,8 @@ public void updateCachedLocations(final TableName tableName, byte[] regionName,
RegionInfo regionInfo = oldLocation.getRegion();
Throwable cause = ClientExceptionsUtil.findException(exception);
if (cause != null) {
if (!ClientExceptionsUtil.isMetaClearingException(cause)) {
if (!ClientExceptionsUtil.isMetaClearingException(cause)
|| ClientExceptionsUtil.isRegionServerOverloadedException(cause)) {
// We know that the region is still on this region server
return;
}
Expand Down
31 changes: 20 additions & 11 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.hadoop.hbase.CallDroppedException;
import org.apache.hadoop.hbase.CallQueueTooBigException;
import org.apache.hadoop.hbase.DoNotRetryIOException;
import org.apache.hadoop.hbase.MultiActionResultTooLarge;
import org.apache.hadoop.hbase.NotServingRegionException;
import org.apache.hadoop.hbase.RegionTooBusyException;
import org.apache.hadoop.hbase.RetryImmediatelyException;
Expand All @@ -59,18 +58,28 @@ public static boolean isMetaClearingException(Throwable cur) {
if (cur == null) {
return true;
}
return !isSpecialException(cur) || (cur instanceof RegionMovedException)
|| cur instanceof NotServingRegionException;
return !isMetaCachePreservingException(cur);
}

public static boolean isSpecialException(Throwable cur) {
return (cur instanceof RegionMovedException || cur instanceof RegionOpeningException
|| cur instanceof RegionTooBusyException || cur instanceof RpcThrottlingException
|| cur instanceof MultiActionResultTooLarge || cur instanceof RetryImmediatelyException
|| cur instanceof CallQueueTooBigException || cur instanceof CallDroppedException
|| cur instanceof NotServingRegionException || cur instanceof RequestTooBigException);
public static boolean isRegionServerOverloadedException(Throwable t) {
t = findException(t);
return isInstanceOfRegionServerOverloadedException(t);
}

private static boolean isInstanceOfRegionServerOverloadedException(Throwable t) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good namings.

return t instanceof CallQueueTooBigException || t instanceof CallDroppedException;
}

private static boolean isMetaCachePreservingException(Throwable t) {
return t instanceof RegionOpeningException || t instanceof RegionTooBusyException
|| t instanceof RpcThrottlingException || t instanceof RetryImmediatelyException
|| t instanceof RequestTooBigException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this list here is smaller than the list above in the old isSpecialException. I looked at one or two of the differences. It seems like superclasses are listed here and you drop the mention of subclasses. Is that the case? Just a little worried we're not catching something here that we used to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just remove subclasses, you mean the detailed messages in the subclass?

}

private static boolean isExceptionWeCare(Throwable t) {
return isMetaCachePreservingException(t) || isInstanceOfRegionServerOverloadedException(t)
|| t instanceof NotServingRegionException;
}

/**
* Look for an exception we know in the remote exception:
Expand All @@ -87,15 +96,15 @@ public static Throwable findException(Object exception) {
}
Throwable cur = (Throwable) exception;
while (cur != null) {
if (isSpecialException(cur)) {
if (isExceptionWeCare(cur)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Good. isSpecialException is an awful name. isExceptionWeCare is too but you made it private and it is only used here so its ok.

return cur;
}
if (cur instanceof RemoteException) {
RemoteException re = (RemoteException) cur;
cur = re.unwrapRemoteException();

// unwrapRemoteException can return the exception given as a parameter when it cannot
// unwrap it. In this case, there is no need to look further
// unwrap it. In this case, there is no need to look further
// noinspection ObjectEquality
if (cur == re) {
return cur;
Expand Down
10 changes: 8 additions & 2 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestMetaCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.ArrayList;
import java.util.List;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.CallDroppedException;
import org.apache.hadoop.hbase.CallQueueTooBigException;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
Expand All @@ -40,6 +41,7 @@
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil;
import org.apache.hadoop.hbase.exceptions.RegionOpeningException;
import org.apache.hadoop.hbase.exceptions.RequestTooBigException;
import org.apache.hadoop.hbase.quotas.RpcThrottlingException;
import org.apache.hadoop.hbase.regionserver.HRegionServer;
import org.apache.hadoop.hbase.regionserver.RSRpcServices;
Expand Down Expand Up @@ -143,12 +145,14 @@ public void testPreserveMetaCacheOnException() throws Exception {
table.mutateRow(mutations);
} catch (IOException ex) {
// Only keep track of the last exception that updated the meta cache
if (ClientExceptionsUtil.isMetaClearingException(ex) || success) {
if ((ClientExceptionsUtil.isMetaClearingException(ex)
&& !ClientExceptionsUtil.isRegionServerOverloadedException(ex)) || success) {
exp = ex;
}
}
// Do not test if we did not touch the meta cache in this iteration.
if (exp != null && ClientExceptionsUtil.isMetaClearingException(exp)) {
if (exp != null && ClientExceptionsUtil.isMetaClearingException(exp)
&& !ClientExceptionsUtil.isRegionServerOverloadedException(exp)) {
assertNull(conn.getCachedLocation(TABLE_NAME, row));
} else if (success) {
assertNotNull(conn.getCachedLocation(TABLE_NAME, row));
Expand Down Expand Up @@ -207,7 +211,9 @@ public static List<Throwable> metaCachePreservingExceptions() {
add(new RpcThrottlingException(" "));
add(new MultiActionResultTooLarge(" "));
add(new RetryImmediatelyException(" "));
add(new RequestTooBigException());
add(new CallQueueTooBigException());
add(new CallDroppedException());
}};
}

Expand Down