Skip to content

[bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down#10348

Merged
Jackie-Jiang merged 2 commits intoapache:masterfrom
aidar-stripe:aidar-st-percent_segments_available_fix
Mar 1, 2023
Merged

[bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down#10348
Jackie-Jiang merged 2 commits intoapache:masterfrom
aidar-stripe:aidar-st-percent_segments_available_fix

Conversation

@aidar-stripe
Copy link
Contributor

Description

For tables that have high number of segments PERCENT_SEGMENTS_AVAILABLE reports 100% availability despite a few replicas being offline. Caused by rounding down (nOffline * 100 / nSegments) to int before subtraction from 100.

This PR changes metric logic to properly round down PERCENT_SEGMENTS_AVAILABLE and report 99% availability in such scenarios.

Testing

  • Added unit test to verify

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.

Good catch. Can you take a look at the test failures?

…ix/SegmentStatusChecker.java

Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>
@aidar-stripe
Copy link
Contributor Author

Good catch. Can you take a look at the test failures?

I'm not quite sure why some tests end up failing, it's also different set of tests that fail when I run maven locally. Let's see how successful they are after this change.

@codecov-commenter
Copy link

Codecov Report

Merging #10348 (c6adede) into master (3ba6d26) will increase coverage by 38.28%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master   #10348       +/-   ##
=============================================
+ Coverage     32.05%   70.33%   +38.28%     
- Complexity      236     5940     +5704     
=============================================
  Files          2029     2035        +6     
  Lines        109970   110228      +258     
  Branches      16711    16748       +37     
=============================================
+ Hits          35253    77532    +42279     
+ Misses        71563    27270    -44293     
- Partials       3154     5426     +2272     
Flag Coverage Δ
integration1 24.48% <0.00%> (-0.06%) ⬇️
integration2 24.47% <0.00%> (?)
unittests1 67.77% <ø> (?)
unittests2 13.76% <0.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...e/pinot/controller/helix/SegmentStatusChecker.java 89.50% <0.00%> (+1.65%) ⬆️
...elix/core/periodictask/ControllerPeriodicTask.java 66.07% <0.00%> (-5.36%) ⬇️
...ntroller/helix/core/minion/TaskMetricsEmitter.java 88.46% <0.00%> (-3.85%) ⬇️
...not/broker/broker/helix/ClusterChangeMediator.java 75.26% <0.00%> (-3.23%) ⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java 54.20% <0.00%> (-1.87%) ⬇️
.../helix/core/realtime/SegmentCompletionManager.java 72.15% <0.00%> (-0.82%) ⬇️
...lix/core/minion/PinotHelixTaskResourceManager.java 50.78% <0.00%> (-0.23%) ⬇️
...pache/pinot/core/query/utils/idset/EmptyIdSet.java 25.00% <0.00%> (ø)
.../java/org/apache/pinot/client/PinotConnection.java 66.66% <0.00%> (ø)
...t/core/operator/filter/MatchAllFilterOperator.java 100.00% <0.00%> (ø)
... and 1276 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Jackie-Jiang Jackie-Jiang merged commit c58a01d into apache:master Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants