You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "aidar-st (via GitHub)" <gi...@apache.org> on 2023/02/28 00:08:44 UTC
[GitHub] [pinot] aidar-st opened a new pull request, #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
aidar-st opened a new pull request, #10348:
URL: https://github.com/apache/pinot/pull/10348
# 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10348:
URL: https://github.com/apache/pinot/pull/10348#issuecomment-1449237864
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10348](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c6adede) into [master](https://codecov.io/gh/apache/pinot/commit/3ba6d2682b05b6920baee5ccf80103678dea18e7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3ba6d26) will **increase** coverage by `38.28%`.
> The diff coverage is `0.00%`.
```diff
@@ 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%)` | :arrow_down: |
| integration2 | `24.47% <0.00%> (?)` | |
| unittests1 | `67.77% <ø> (?)` | |
| unittests2 | `13.76% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/pinot/controller/helix/SegmentStatusChecker.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9TZWdtZW50U3RhdHVzQ2hlY2tlci5qYXZh) | `89.50% <0.00%> (+1.65%)` | :arrow_up: |
| [...elix/core/periodictask/ControllerPeriodicTask.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3BlcmlvZGljdGFzay9Db250cm9sbGVyUGVyaW9kaWNUYXNrLmphdmE=) | `66.07% <0.00%> (-5.36%)` | :arrow_down: |
| [...ntroller/helix/core/minion/TaskMetricsEmitter.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9UYXNrTWV0cmljc0VtaXR0ZXIuamF2YQ==) | `88.46% <0.00%> (-3.85%)` | :arrow_down: |
| [...not/broker/broker/helix/ClusterChangeMediator.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0NsdXN0ZXJDaGFuZ2VNZWRpYXRvci5qYXZh) | `75.26% <0.00%> (-3.23%)` | :arrow_down: |
| [...r/helix/SegmentOnlineOfflineStateModelFactory.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvc3RhcnRlci9oZWxpeC9TZWdtZW50T25saW5lT2ZmbGluZVN0YXRlTW9kZWxGYWN0b3J5LmphdmE=) | `54.20% <0.00%> (-1.87%)` | :arrow_down: |
| [.../helix/core/realtime/SegmentCompletionManager.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1NlZ21lbnRDb21wbGV0aW9uTWFuYWdlci5qYXZh) | `72.15% <0.00%> (-0.82%)` | :arrow_down: |
| [...lix/core/minion/PinotHelixTaskResourceManager.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL21pbmlvbi9QaW5vdEhlbGl4VGFza1Jlc291cmNlTWFuYWdlci5qYXZh) | `50.78% <0.00%> (-0.23%)` | :arrow_down: |
| [...pache/pinot/core/query/utils/idset/EmptyIdSet.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS91dGlscy9pZHNldC9FbXB0eUlkU2V0LmphdmE=) | `25.00% <0.00%> (ø)` | |
| [.../java/org/apache/pinot/client/PinotConnection.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qZGJjLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Bpbm90Q29ubmVjdGlvbi5qYXZh) | `66.66% <0.00%> (ø)` | |
| [...t/core/operator/filter/MatchAllFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvTWF0Y2hBbGxGaWx0ZXJPcGVyYXRvci5qYXZh) | `100.00% <0.00%> (ø)` | |
| ... and [1276 more](https://codecov.io/gh/apache/pinot/pull/10348?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10348:
URL: https://github.com/apache/pinot/pull/10348
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] aidar-st commented on pull request #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
Posted by "aidar-st (via GitHub)" <gi...@apache.org>.
aidar-st commented on PR #10348:
URL: https://github.com/apache/pinot/pull/10348#issuecomment-1449178243
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10348:
URL: https://github.com/apache/pinot/pull/10348#discussion_r1120594397
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##########
@@ -298,7 +298,7 @@ private void updateSegmentMetrics(String tableNameWithType, TableConfig tableCon
(nReplicasIdealMax > 0) ? (nReplicasExternal * 100 / nReplicasIdealMax) : 100);
_controllerMetrics.setValueOfTableGauge(tableNameWithType, ControllerGauge.SEGMENTS_IN_ERROR_STATE, nErrors);
_controllerMetrics.setValueOfTableGauge(tableNameWithType, ControllerGauge.PERCENT_SEGMENTS_AVAILABLE,
- (nSegments > 0) ? (100 - (nOffline * 100 / nSegments)) : 100);
+ (nSegments > 0) ? (long) (100 - (nOffline * 100.0 / nSegments)) : 100);
Review Comment:
Avoid introducing double to avoid the precision issue
```suggestion
(nSegments > 0) ? (nSegments - nOffline) * 100 / nSegments : 100);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] aidar-st commented on a diff in pull request #10348: [bugfix] Fix bug with PERCENT_SEGMENTS_AVAILABLE metric rounding down
Posted by "aidar-st (via GitHub)" <gi...@apache.org>.
aidar-st commented on code in PR #10348:
URL: https://github.com/apache/pinot/pull/10348#discussion_r1121003259
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##########
@@ -298,7 +298,7 @@ private void updateSegmentMetrics(String tableNameWithType, TableConfig tableCon
(nReplicasIdealMax > 0) ? (nReplicasExternal * 100 / nReplicasIdealMax) : 100);
_controllerMetrics.setValueOfTableGauge(tableNameWithType, ControllerGauge.SEGMENTS_IN_ERROR_STATE, nErrors);
_controllerMetrics.setValueOfTableGauge(tableNameWithType, ControllerGauge.PERCENT_SEGMENTS_AVAILABLE,
- (nSegments > 0) ? (100 - (nOffline * 100 / nSegments)) : 100);
+ (nSegments > 0) ? (long) (100 - (nOffline * 100.0 / nSegments)) : 100);
Review Comment:
Good idea!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org