-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HBase 2.x #2675
Conversation
pankaj72981
commented
Nov 18, 2020
- Added a check for Object class in RegionCoprocessorHost to avoid wrong initialization of hasCustomPostScannerFilterRow
- Removed duplication implementation from AccessController, VisibilityController & ConstraintProcessor (which are not required currently)
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
hbase-server/src/main/java/org/apache/hadoop/hbase/security/access/AccessController.java
Show resolved
Hide resolved
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.
Looks good to me. In the parent JIRA for the perf report how many CPs were exactly added in your tests ? Were they only ACL and visibility controller? Any impact on number of CPs that are getting added. It would be good to mention them too.
Following region CPs were configured.
Yeah scan perf will have impact when number of CPs are more, for each row postScannerFilterRow will be call for each CPs. |
@@ -299,6 +304,11 @@ public RegionCoprocessorHost(final HRegion region, | |||
} catch (NoSuchMethodException ignore) { | |||
} | |||
clazz = clazz.getSuperclass(); | |||
|
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.
Actually change can be at begin of this loop
if (clazz == null) {
// we must have directly implemented RegionObserver
hasCustomPostScannerFilterRow = true;
break out;
}
In 1.x we have interface with no def impl. So we have to consider like hasCustomPostScannerFilterRow = true then. But in 2.x we have Interface with default impl. So its safe to change there to be hasCustomPostScannerFilterRow = false only.
In fact that check can be as urs
if (clazz == Object.class) {
break out;
}
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.
In fact that check can be as urs
if (clazz == Object.class) {
break out;
}
make sense, will update the PR.
dd0ae8d
to
a43d31b
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -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.
Added a minor comment. Else LGTM
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionCoprocessorHost.java
Outdated
Show resolved
Hide resolved
Test issue anyway related? |
Test failures are not relevant. |
6653cee
to
af0dada
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…iple CPs are configured and few CP override postScannerFilterRow
af0dada
to
c612df8
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
init(null); | ||
} | ||
|
||
public void init(Boolean flag) throws IOException { |
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.
Let's use private here? I do not think this method needs to be called in other classes?
@@ -102,6 +102,11 @@ | |||
// optimization: no need to call postScannerFilterRow, if no coprocessor implements it | |||
private final boolean hasCustomPostScannerFilterRow; | |||
|
|||
@VisibleForTesting |
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, VisibleForTesting...
We have a dicussion thread in the mailing list, and @apurtell concluded that we do not use annotation at all, just use a javadoc, and had already done a PR to remove all the VisibleForTesting annotations from our code base. I'm still trying to convince him to allow this annotation on IA.Private classes.
There is still no consensus yet, you can see the discussion in this PR
#2714
So here I suggest that we remove this annotation to align with the current rule in our code base first. But anyway, I think this is an example that developers want to use this annotation, so maybe @apurtell could change his opinion.
Thanks.
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.
Yeah, VisibleForTesting is useless. Will remove this.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
UT failures are not related. |
…ase 2.x (#2675) * HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HBase 2.x 1. Added a check for Object class in RegionCoprocessorHost to avoid wrong initialization of hasCustomPostScannerFilterRow 2. Removed dummy implementation of postScannerFilterRow from AccessController, VisibilityController & ConstraintProcessor (which are not required currently) Signed-off-by Ramkrishna S Vasudevan <ramkrishna@apache.org> Signed-off-by Anoop Sam John <anoopsamjohn@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit fb6e498)
…ase 2.x (#2675) * HBASE-25277 postScannerFilterRow impacts Scan performance a lot in HBase 2.x 1. Added a check for Object class in RegionCoprocessorHost to avoid wrong initialization of hasCustomPostScannerFilterRow 2. Removed dummy implementation of postScannerFilterRow from AccessController, VisibilityController & ConstraintProcessor (which are not required currently) Signed-off-by Ramkrishna S Vasudevan <ramkrishna@apache.org> Signed-off-by Anoop Sam John <anoopsamjohn@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit fb6e498) (cherry picked from commit ec8eb65)