-
Notifications
You must be signed in to change notification settings - Fork 949
[KYUUBI #7079] Improve performance of AccessRequest initialization #7081
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
base: master
Are you sure you want to change the base?
Conversation
* @param argClasses the classes of the arguments | ||
* @return an unbound method that can be invoked later | ||
*/ | ||
def getMethod(clz: Class[_], methodName: String, argClasses: Class[_]*): UnboundMethod = { |
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.
this does not simplify much code, can we inline it?
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.
this does not simplify much code, can we inline it?
Makes sense, changed.
...kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/rule/RuleHelper.scala
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7081 +/- ##
=======================================
Coverage 0.00% 0.00%
=======================================
Files 697 700 +3
Lines 43203 43457 +254
Branches 5854 5887 +33
=======================================
- Misses 43203 43457 +254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
private def getUserGroupsFromUgi(user: UserGroupInformation): JSet[String] = { | ||
user.getGroupNames.toSet.asJava | ||
} | ||
|
||
private def getUserGroupsFromUserStore(user: UserGroupInformation): Option[JSet[String]] = { | ||
private lazy val userGroupMappingOpt: Option[JHashMap[String, JSet[String]]] = { |
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.
I'm not sure if this is safe to cache
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.
I'm not sure if this is safe to cache
userGroupMapping
may be updated by RangerUserStoreRefresher, I will only cache reflect methods
@@ -68,17 +70,43 @@ object AccessRequest { | |||
req | |||
} | |||
|
|||
private val getRolesFromUserAndGroupsMethod: Option[UnboundMethod] = | |||
getMethod( | |||
SparkRangerAdminPlugin.getClass, |
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.
SparkRangerAdminPlugin.getClass, | |
classOf[SparkRangerAdminPlugin], |
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.
SparkRangerAdminPlugin
is an object and does not seem to work with classOf
.
...i-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/AccessRequest.scala
Outdated
Show resolved
Hide resolved
|
||
private lazy val getUserGroupMappingMethod: Option[UnboundMethod] = | ||
getMethod( | ||
Class.forName("org.apache.ranger.plugin.util.RangerUserStore"), |
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.
it's located at ranger-plugins-common
, should be always accessible?
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.
it's located at
ranger-plugins-common
, should be always accessible?
I don't get your point, but the logic here seems to be consistent with previous.
Lines 79 to 80 in 4e40f94
val userGroupMapping = | |
invokeAs[JHashMap[String, JSet[String]]](userStore, "getUserGroupMapping") |
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.
I see, it's a new feature of Ranger 2.1, we must use reflection as long as we support lower Ranger versions
…kyuubi/plugin/spark/authz/ranger/AccessRequest.scala Co-authored-by: Cheng Pan <pan3793@gmail.com>
…kyuubi/plugin/spark/authz/ranger/AccessRequest.scala Co-authored-by: Cheng Pan <pan3793@gmail.com>
…/apache/kyuubi/plugin/spark/authz/ranger/AccessRequest.scala" This reverts commit 686c45b.
…/apache/kyuubi/plugin/spark/authz/ranger/AccessRequest.scala" This reverts commit 99e0bd3.
Why are the changes needed?
Improve performance of authz rules.
Constantize some properties to reduce object creation:
RuleHelper.ugi(different sparkSession may have different authz ugi)closes #7079
How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?
No