feat: Cache BCrypt checkpw to enhance pinot query performance #9636
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9636 +/- ##
=============================================
+ Coverage 28.06% 68.61% +40.55%
- Complexity 53 4848 +4795
=============================================
Files 1935 1947 +12
Lines 103815 104182 +367
Branches 15757 15798 +41
=============================================
+ Hits 29134 71486 +42352
+ Misses 71808 27589 -44219
- Partials 2873 5107 +2234
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
thank you for the contribution. I haven't look at the cache logic but could we consolidate the 3 access control factory first? thank you!
| .filter(entry -> BcryptUtils.checkpwWithCache(entry.getKey(), entry.getValue().getPassword(), | ||
| _userCache.getUserPasswordAuthCache())) | ||
| .map(u -> u.getValue()).filter(Objects::nonNull).findFirst(); |
There was a problem hiding this comment.
i dont know what's the relationship between this and the pinot-controller/src/main/java/org/apache/pinot/controller/api/access/ZkBasicAuthAccessControlFactory.java. please make sure both side are consistent
There was a problem hiding this comment.
That's fine , It would be clear to use ZkBasicAuthAccessFactory
There was a problem hiding this comment.
yeah please consolidate the usage into just one. this will make future maintenance easier
pinot-common/src/main/java/org/apache/pinot/common/config/provider/AccessControlUserCache.java
Outdated
Show resolved
Hide resolved
| .collect(Collectors.toMap(name2password::get, _name2principal::get)); | ||
| return password2principal.entrySet().stream() | ||
| .filter(entry -> BcryptUtils.checkpw(entry.getKey(), entry.getValue().getPassword())) | ||
| .filter(entry -> BcryptUtils.checkpwWithCache(entry.getKey(), entry.getValue().getPassword(), |
There was a problem hiding this comment.
name it consistent with the broker one.
also why do we need 3? can't we just keep one in core?
There was a problem hiding this comment.
Thx for review, Do you means we need merge
org.apache.pinot.controller.api.access.ZkBasicAuthAccessControlFactory
org.apache.pinot.broker.broker.ZkBasicAuthAccessControlFactory
to
org.apache.pinot.server.access.ZkBasicAuthAccessFactory as hole project ZkBasicAuthAccessFactory
for Controller, broker and server nodes
There was a problem hiding this comment.
yes unless there's a specific reason controll/broker/server needs 3 different auth factory (and cannot be achieved via config differences)
There was a problem hiding this comment.
Thx @walterddr But if we need to merge controller, broker, server ZkBasicAuthAccessFactory,
Should we merge user and password for controller, broker, server nodes as well?
In other words controller, broker and server nodes should use same user and password for hole pinot cluster
…ider/AccessControlUserCache.java Co-authored-by: Rong Rong <rongr@apache.org>
|
Thanks for the contribution! Please reformat the changes with Pinot Style |
4 participants
Instructions:
The PR has to be tagged with at least one of the following labels (*):
performancePurpose for this pull request:
We found a performance issue from master pinot code
When we use zk as user and password authorization we found this function spend about 80ms for verifying
isMatch = BCrypt.checkpw(pasword, encrypedPassword)So this solution is to cache this function result to enhance query speed
Here is testing result in our lab env:
Original code need 85ms at least to query
Fixed code need 1 - 10ms at least to query
Related issues: Cache BCrypt checkpw to enahce query performance #9632