-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Conversation
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question. Otherwise, this LGTM.
@@ -87,15 +96,15 @@ public static Throwable findException(Object exception) { | |||
} | |||
Throwable cur = (Throwable) exception; | |||
while (cur != null) { | |||
if (isSpecialException(cur)) { | |||
if (isExceptionWeCare(cur)) { |
There was a problem hiding this comment.
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.
private static boolean isMetaCachePreservingException(Throwable t) { | ||
return t instanceof RegionOpeningException || t instanceof RegionTooBusyException | ||
|| t instanceof RpcThrottlingException || t instanceof RetryImmediatelyException | ||
|| t instanceof RequestTooBigException; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
return isInstanceOfRegionServerOverloadedException(t); | ||
} | ||
|
||
private static boolean isInstanceOfRegionServerOverloadedException(Throwable t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good namings.
Pulled back to branch-2.2 and branch-2.1 too as well as branch-2. Doesn't look like it makes sense on master. If not, all is good else, open new issue to forward parth. Thanks @johnhomsea . |
(cherry picked from commit b1d4878) Change-Id: I0cee17d3d5bb80ddb880be07f0edab9ef7a8b890
No description provided.