pinot icon indicating copy to clipboard operation
pinot copied to clipboard

Remove deprecated commons-lang3 RandomUtils class usage.

Open abhioncbr opened this issue 1 year ago • 3 comments

Labels

  • refactor
  • cleanup

Description

  • As per the comment of the PR, This PR removes the usage of commons-lang3 RandomUtils class.
  • As per the deprecation suggestions, We have introduced the commons-rng dependency in Pinot.
  • Added a RandomNumberUtils class to generate random numbers based on the commons-rng.

abhioncbr avatar Jul 02 '24 15:07 abhioncbr

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.09%. Comparing base (59551e4) to head (b267621). Report is 731 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #13525      +/-   ##
============================================
+ Coverage     61.75%   62.09%   +0.33%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2558     +122     
  Lines        133233   140905    +7672     
  Branches      20636    21865    +1229     
============================================
+ Hits          82274    87489    +5215     
- Misses        44911    46792    +1881     
- Partials       6048     6624     +576     
Flag Coverage Δ
custom-integration1 <0.01% <0.00%> (-0.01%) :arrow_down:
integration <0.01% <0.00%> (-0.01%) :arrow_down:
integration1 <0.01% <0.00%> (-0.01%) :arrow_down:
integration2 0.00% <0.00%> (ø)
java-11 62.06% <100.00%> (+0.35%) :arrow_up:
java-21 61.97% <100.00%> (+0.34%) :arrow_up:
skip-bytebuffers-false 62.08% <100.00%> (+0.33%) :arrow_up:
skip-bytebuffers-true 61.95% <100.00%> (+34.22%) :arrow_up:
temurin 62.09% <100.00%> (+0.33%) :arrow_up:
unittests 62.08% <100.00%> (+0.33%) :arrow_up:
unittests1 46.65% <ø> (-0.24%) :arrow_down:
unittests2 27.66% <100.00%> (-0.07%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 02 '24 15:07 codecov-commenter

Seems all the usages are within the tests, and this library is not that commonly used (only 71 usages). I'd suggest just using regular java Random() for readability

I agree that this library isn't widely used and that releases are minimal, but we can use it if we are looking for performance gains. Here are the stats related to performance. Based on these stats, do you think we should try to replace java.util.Random usage in our entire codebase with this?

abhioncbr avatar Jul 02 '24 23:07 abhioncbr

We rarely use random in production code (especially in high perf code), so I'd suggest just using Random for now and revisit if we need high performance random generator

Jackie-Jiang avatar Jul 03 '24 00:07 Jackie-Jiang