[KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication #6758

wankunde · 2024-10-18T10:26:48Z

🔍 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 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Add benchmark
Before

Java HotSpot(TM) 64-Bit Server VM 17.0.12+8-LTS-286 on Mac OS X 14.6
Apple M3
Collecting files ranger access request:   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
50000 files benchmark                            181863         189434         NaN         -0.0 -181863368958.0       1.0X

Behavior With This Pull Request 🎉

After

Java HotSpot(TM) 64-Bit Server VM 17.0.12+8-LTS-286 on Mac OS X 14.6
Apple M3
Collecting files ranger access request:   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
50000 files benchmark                              1281           1310          33         -0.0 -1280563000.0       1.0X

Related Unit Tests

Exists UT

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

extensions/spark/kyuubi-spark-authz/pom.xml

pan3793 · 2024-10-18T13:12:45Z

...ark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala

@@ -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]()


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

and please leave a few comments here to explain the background

and, was immutable.Map.newBuilder considered?

Change to mutable.HashMap

Added some comments

I'm not sure if immutable.Map.newBuilder is better than mutable.HashMap?

How about using mutable.LinkedHashSet[AccessRequest] as the type for requests?

it maintains the original adding order

it guarantees the element's uniqueness reliing on the hashCode

good for element looping as fast as the array

And make sure related methods of AccessRequest properly implemented. Add some unit test for assuring it.

The uniqueness is guaranteed by hashCode, which could be told from its name HashSet of LinkedHashSet. Not much overhead cost for maintaining the order inside, just adding a reference to a linked list. And the linked list perform well in looping elements.

I mean mutable.LinkedHashSet[AccessRequest] may be slower than HashSet + ArrayBuffer in our case.

the unique key is NOT the AccessRequest itself but part of properties, the combination of ArrayBuffer with a HashSet looks fine to me.

I mean mutable.LinkedHashSet[AccessRequest] may be slower than HashSet + ArrayBuffer in our case.

No, it wont be slowed down, as LinkedHashSet maintaining both HashSet and Linked list inside.

the unique key is NOT the AccessRequest itself but part of properties, the combination of ArrayBuffer with a HashSet looks fine to me.

Noticed. But the HashSet still reily on the same logic with hashCode inside. And no proper customed method of either AccessRequest or its parent class.

pan3793 · 2024-10-18T13:14:19Z

...uthz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorizationSuite.scala

+  }
+
+  // scalastyle:on
+  test("KYUUBI #6754: improve the performance of ranger access requests") {


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?

Added benchmark, thanks

Co-authored-by: Cheng Pan <[email protected]>

codecov-commenter · 2024-10-20T11:52:40Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (2d64255) to head (9d7d196).
Report is 32 commits behind head on master.

Files with missing lines	Patch %	Lines
.../plugin/spark/authz/ranger/RuleAuthorization.scala	0.00%	4 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #6758   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         684     684           
  Lines       42282   42289    +7     
  Branches     5767    5769    +2     
======================================
- Misses      42282   42289    +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pan3793

thanks, lgtm, only a few comments

extensions/spark/kyuubi-spark-authz/pom.xml

...ark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala

...park/kyuubi-spark-authz/src/test/scala/org/apache/spark/sql/RuleAuthorizationBenchmark.scala

Co-authored-by: Cheng Pan <[email protected]>

…spark/sql/RuleAuthorizationBenchmark.scala Co-authored-by: Cheng Pan <[email protected]>

bowenliang123

LGTM. Thanks for the contribution.

bowenliang123 · 2024-10-21T13:18:25Z

Thanks for your contribution! Merged to master (1.10.0) .

[KYUUBI apache#6754] Improve the performance of ranger access requests

e198179

github-actions bot added module:spark kind:build module:extensions module:authz labels Oct 18, 2024

pan3793 reviewed Oct 18, 2024

View reviewed changes

wankunde and others added 5 commits October 19, 2024 12:17

Update extensions/spark/kyuubi-spark-authz/pom.xml

805e8a9

Co-authored-by: Cheng Pan <[email protected]>

Update extensions/spark/kyuubi-spark-authz/pom.xml

01e15c1

Co-authored-by: Cheng Pan <[email protected]>

[KYUUBI apache#6754] Improve the performance of ranger access requests

1374604

[KYUUBI apache#6754] Improve the performance of ranger access requests

aad08a6

[KYUUBI apache#6754] Improve the performance of ranger access requests

b904b49

wankunde changed the title ~~[KYUUBI #6754] Improve the performance of ranger access requests~~ [WIP][KYUUBI #6754] Improve the performance of ranger access requests Oct 20, 2024

[KYUUBI apache#6754] Improve the performance of ranger access requests

d86b01f

wankunde changed the title ~~[WIP][KYUUBI #6754] Improve the performance of ranger access requests~~ [KYUUBI #6754] Improve the performance of ranger access requests Oct 20, 2024

wankunde changed the title ~~[KYUUBI #6754] Improve the performance of ranger access requests~~ [WIP][KYUUBI #6754] Improve the performance of ranger access requests Oct 20, 2024

wankunde added 2 commits October 20, 2024 18:31

[KYUUBI apache#6754] Improve the performance of ranger access requests

9793249

[KYUUBI apache#6754] Improve the performance of ranger access requests

f5a3b6c

wankunde changed the title ~~[WIP][KYUUBI #6754] Improve the performance of ranger access requests~~ [KYUUBI #6754] Improve the performance of ranger access requests Oct 20, 2024

pan3793 approved these changes Oct 21, 2024

View reviewed changes

wankunde and others added 3 commits October 21, 2024 11:03

Update extensions/spark/kyuubi-spark-authz/pom.xml

20c55fb

Co-authored-by: Cheng Pan <[email protected]>

Update extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/…

88b9c04

…spark/sql/RuleAuthorizationBenchmark.scala Co-authored-by: Cheng Pan <[email protected]>

[KYUUBI apache#6754] Improve the performance of ranger access requests

9d7d196

bowenliang123 approved these changes Oct 21, 2024

View reviewed changes

bowenliang123 requested a review from yaooqinn October 21, 2024 09:34

bowenliang123 assigned wankunde Oct 21, 2024

bowenliang123 changed the title ~~[KYUUBI #6754] Improve the performance of ranger access requests~~ [KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication Oct 21, 2024

bowenliang123 added this to the v1.10.0 milestone Oct 21, 2024

bowenliang123 closed this in 04f4437 Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication #6758

[KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication #6758

wankunde commented Oct 18, 2024 •

edited

Loading

pan3793 Oct 18, 2024 •

edited

Loading

pan3793 Oct 18, 2024

pan3793 Oct 18, 2024

wankunde Oct 19, 2024

bowenliang123 Oct 21, 2024

bowenliang123 Oct 21, 2024

wankunde Oct 21, 2024

pan3793 Oct 21, 2024

bowenliang123 Oct 21, 2024

bowenliang123 Oct 21, 2024

pan3793 Oct 18, 2024

wankunde Oct 19, 2024

codecov-commenter commented Oct 20, 2024 •

edited

Loading

pan3793 left a comment

bowenliang123 left a comment

bowenliang123 commented Oct 21, 2024

[KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication #6758

[KYUUBI #6754][AUTHZ] Improve the performance of Ranger access requests deduplication #6758

Conversation

wankunde commented Oct 18, 2024 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

pan3793 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 20, 2024 • edited Loading

Codecov Report

pan3793 left a comment

Choose a reason for hiding this comment

bowenliang123 left a comment

Choose a reason for hiding this comment

bowenliang123 commented Oct 21, 2024

wankunde commented Oct 18, 2024 •

edited

Loading

pan3793 Oct 18, 2024 •

edited

Loading

codecov-commenter commented Oct 20, 2024 •

edited

Loading