-
Notifications
You must be signed in to change notification settings - Fork 912
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
[KYUUBI #6754] Improve the performance of ranger access requests #6758
base: master
Are you sure you want to change the base?
Conversation
@@ -34,23 +35,22 @@ case class RuleAuthorization(spark: SparkSession) extends Authorization(spark) { | |||
val auditHandler = new SparkRangerAuditHandler | |||
val ugi = getAuthzUgi(spark.sparkContext) | |||
val (inputs, outputs, opType) = PrivilegesBuilder.build(plan, spark) | |||
val requests = new ArrayBuffer[AccessRequest]() | |||
var requests = new HashMap[(AccessResource, AccessType), AccessRequest]() |
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.
Why immutable.HashMap
instead of mutable.HashMap
is used here?
BTW, Scala 2.13 changed the alias of HashMap
from mutable.HashMap
to immutable.HashMap
, to make the code clear, please write new immutable.HashMap
or new mutable.HashMap
inline
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.
and please leave a few comments here to explain the background
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.
and, was immutable.Map.newBuilder
considered?
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.
- Change to
mutable.HashMap
- Added some comments
- I'm not sure if
immutable.Map.newBuilder
is better thanmutable.HashMap
?
} | ||
|
||
// scalastyle:on | ||
test("KYUUBI #6754: improve the performance of ranger access requests") { |
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.
benchmark cases are not expected to run in regular test workflow, I remember we ported Spark benchmark framework, can we write it in that style?
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 benchmark, thanks
Co-authored-by: Cheng Pan <pan3793@gmail.com>
Co-authored-by: Cheng Pan <pan3793@gmail.com>
🔍 Description
Issue References 🔗
This pull request fixes #6754
Describe Your Solution 🔧
Right now in RuleAuthorization we use an ArrayBuffer to collect access requests, which is very slow because each new PrivilegeObject needs to be compared with all access requests.
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Add benchmark
Before
After
Checklist 📝
Be nice. Be informative.