Skip to content
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

Improve the retry policy so that the thread doesn't sleep after the last failure #9158

Conversation

jasperjiaguo
Copy link
Contributor

@jasperjiaguo jasperjiaguo commented Aug 3, 2022

in the previous code, if I set _maxNumAttempts = 3 the program will do

action attempt
try 0
sleep 1
try 1
sleep 2
try 2
sleep 3
break

For failed attempts, where the last sleep is causing unnecessary hanging especially for exponential back off case. The new code will be

action attempt
try 0
sleep 1
try 1
sleep 2
break
try

@jasperjiaguo jasperjiaguo changed the title Improve the retry policy so that the thread don't sleep after the last failure Improve the retry policy so that the thread doesn't sleep after the last failure Aug 3, 2022
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #9158 (0e9be14) into master (dad2586) will increase coverage by 6.40%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master    #9158      +/-   ##
============================================
+ Coverage     63.48%   69.88%   +6.40%     
- Complexity     4970     4998      +28     
============================================
  Files          1799     1848      +49     
  Lines         95706    98478    +2772     
  Branches      14492    14938     +446     
============================================
+ Hits          60760    68824    +8064     
+ Misses        30574    24815    -5759     
- Partials       4372     4839     +467     
Flag Coverage Δ
integration1 26.33% <0.00%> (?)
integration2 24.69% <0.00%> (?)
unittests1 66.98% <60.00%> (+0.01%) ⬆️
unittests2 15.28% <0.00%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
.../apache/pinot/spi/utils/retry/BaseRetryPolicy.java 85.71% <60.00%> (-14.29%) ⬇️
...nt/local/startree/v2/store/StarTreeDataSource.java 40.00% <0.00%> (-13.34%) ⬇️
...y/aggregation/function/AvgAggregationFunction.java 81.67% <0.00%> (-12.99%) ⬇️
...ot/segment/local/startree/OffHeapStarTreeNode.java 65.90% <0.00%> (-6.82%) ⬇️
...rg/apache/pinot/spi/config/table/UpsertConfig.java 80.00% <0.00%> (-5.72%) ⬇️
...core/query/reduce/AggregationDataTableReducer.java 80.95% <0.00%> (-5.33%) ⬇️
...he/pinot/segment/local/segment/store/IndexKey.java 70.00% <0.00%> (-5.00%) ⬇️
...troller/helix/core/retention/RetentionManager.java 79.83% <0.00%> (-4.84%) ⬇️
...tion/function/SumPrecisionAggregationFunction.java 30.58% <0.00%> (-3.24%) ⬇️
...lix/core/realtime/PinotRealtimeSegmentManager.java 76.43% <0.00%> (-2.62%) ⬇️
... and 452 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@siddharthteotia siddharthteotia merged commit e8a8684 into apache:master Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants