specify how many segments were pruned by each server segment pruner by gortiz · Pull Request #8884 · apache/pinot

gortiz · 2022-06-13T11:55:46Z

This PR adds three new metrics that break down the older (and still supported) numSegmentsPrunedByServer.

The three metrics are:

numSegmentsPrunedInvalid: Which include both segments that have been pruned because they are actually invalid (aka having docs) or their physical schema doesn't contain all the required columns (quite common when a new column is added but segments were not reloaded)
numSegmentsPrunedByLimit: The ones pruned by SelectionQuerySegmentPruner, for example: select * from Table limit 10
numSegmentsPrunedByValue: The ones pruned by ColumnValueSegmentPruner, for example when segments can be pruned by using bloom filters or max/min of each column.

These metrics are shown when the controller is used. It may also be interesting to add a warning in the controller when numSegmentsPrunedInvalid is different than 0.

Initially there is only one warning, which is shown when segments were pruned because they were invalid

codecov-commenter · 2022-06-14T07:47:29Z

Codecov Report

Merging #8884 (c376da9) into master (3ff4d13) will decrease coverage by 8.61%.
The diff coverage is 89.10%.

❗ Current head c376da9 differs from pull request most recent head 5c6ef86. Consider uploading reports for the commit 5c6ef86 to get more accurate results

@@             Coverage Diff              @@
##             master    #8884      +/-   ##
============================================
- Coverage     70.09%   61.48%   -8.62%     
- Complexity     4732     4769      +37     
============================================
  Files          1826     1815      -11     
  Lines         95980    95711     -269     
  Branches      14352    14318      -34     
============================================
- Hits          67281    58850    -8431     
- Misses        24057    32531    +8474     
+ Partials       4642     4330     -312

Flag	Coverage Δ
integration1	`26.43% <87.12%> (+0.01%)`	⬆️
integration2	`?`
unittests1	`66.87% <89.10%> (+0.05%)`	⬆️
unittests2	`?`

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

Impacted Files	Coverage Δ
...e/pinot/core/query/config/SegmentPrunerConfig.java	`100.00% <ø> (ø)`
.../pinot/core/query/pruner/SegmentPrunerService.java	`83.33% <69.23%> (-16.67%)`	⬇️
...che/pinot/core/query/scheduler/QueryScheduler.java	`84.02% <83.33%> (-1.25%)`	⬇️
...a/org/apache/pinot/common/metrics/ServerMeter.java	`100.00% <100.00%> (ø)`
...t/common/response/broker/BrokerResponseNative.java	`96.27% <100.00%> (+0.30%)`	⬆️
.../java/org/apache/pinot/common/utils/DataTable.java	`95.45% <100.00%> (+0.33%)`	⬆️
...core/query/executor/ServerQueryExecutorV1Impl.java	`82.72% <100.00%> (-0.22%)`	⬇️
...pinot/core/query/pruner/SegmentPrunerProvider.java	`84.61% <100.00%> (ø)`
...not/core/query/pruner/SegmentPrunerStatistics.java	`100.00% <100.00%> (ø)`
...che/pinot/core/query/reduce/BaseReduceService.java	`94.47% <100.00%> (+0.39%)`	⬆️
... and 348 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ff4d13...5c6ef86. Read the comment docs.

…ents

xiangfu0

overall looks good

walterddr

looks good to me. thanks for adding this support and making it available for users to diagnose invalid segments.
is it possible to add a test for SegmentPruner for this behavior by mocking an invalid segment?

pinot-controller/src/main/resources/app/pages/Query.tsx

Jackie-Jiang

It looks good in general. My only concern is around the empty segment, which IMO shouldn't be count as invalid. Empty segment is normal for certain streaming types e.g. Kinesis. If we emit warning for empty segment, all the query will get the warning, which is undesired

Co-authored-by: Rong Rong <walterddr.walterddr@gmail.com>

gortiz · 2022-06-22T07:42:17Z

My only concern is around the empty segment, which IMO shouldn't be count as invalid.

Changed

is it possible to add a test for SegmentPruner for this behavior by mocking an invalid segment?

I've added some tests inspired by the other SegmentPruner tests.

Jackie-Jiang · 2022-06-23T22:11:25Z

pinot-common/src/main/java/org/apache/pinot/common/utils/DataTable.java

    SYSTEM_ACTIVITIES_CPU_TIME_NS("systemActivitiesCpuTimeNs", MetadataValueType.LONG),
    RESPONSE_SER_CPU_TIME_NS("responseSerializationCpuTimeNs", MetadataValueType.LONG),
    NUM_SEGMENTS_PRUNED_BY_SERVER("numSegmentsPrunedByServer", MetadataValueType.INT),
+    NUM_SEGMENTS_PRUNED_INVALID("numSegmentsPrunedByInvalid", MetadataValueType.INT),


(MAJOR) Please append the new keys to the end. See the javadoc for this enum.
We should associate an id to each key instead of relying on the ordinal of the enum. That is out of the scope of this PR, and we need a newer version data table so that the change is backward compatible

Jackie-Jiang · 2022-06-23T22:16:57Z

pinot-core/src/main/java/org/apache/pinot/core/query/pruner/SegmentPrunerService.java

+    int invalid = 0;
    for (IndexSegment segment : segments) {
-      if (!isInvalidSegment(segment, query)) {
+      boolean isInvalid = isInvalidSegment(segment, query);


(minor) We can skip the valid check if a segment is empty

Suggested change

boolean isInvalid = isInvalidSegment(segment, query);

if (!isEmptySegment(segment)) {

if (isInvalidSegment(segment, query)) {

invalid++

} else {

segments.set(selected++, segment);

}

}

…gmentPrunerService.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

gortiz

Moved new literals to the end of the enum list

gortiz added 5 commits June 13, 2022 13:47

specify how many segments were pruned by each server segment pruner

294e8a4

force the query log when there are invalid segments

2fe644d

change header to javadoc

b5d01ed

Add a warning section in Controller UI query view.

ade5d63

Initially there is only one warning, which is shown when segments were pruned because they were invalid

Correct an assertion

b169c83

Merge remote-tracking branch 'origin/master' into report-invalid-segm…

2a23365

…ents

xiangfu0 approved these changes Jun 15, 2022

View reviewed changes

walterddr approved these changes Jun 15, 2022

View reviewed changes

pinot-controller/src/main/resources/app/pages/Query.tsx Outdated Show resolved Hide resolved

Jackie-Jiang reviewed Jun 16, 2022

View reviewed changes

gortiz and others added 3 commits June 20, 2022 13:43

Apply suggestions from code review

1016d42

Co-authored-by: Rong Rong <walterddr.walterddr@gmail.com>

Don't report empty segments as invalid

08e306d

Add test on SegmentPrunerService

6e25dfe

Jackie-Jiang reviewed Jun 23, 2022

View reviewed changes

Update pinot-core/src/main/java/org/apache/pinot/core/query/pruner/Se…

f813d88

…gmentPrunerService.java Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

gortiz commented Jun 24, 2022

View reviewed changes

gortiz added 5 commits June 24, 2022 09:11

Move new literals to the end of the enum list

4fb64c3

Correctly apply suggested change

0eb632c

Merge branch 'master' into report-invalid-segments

38eb340

remove trailing whitespace

e3b7c55

fix a compilation error

5c6ef86

walterddr merged commit de16a0a into apache:master Jul 5, 2022

Jackie-Jiang mentioned this pull request Aug 4, 2022

Add id to MetadataKey so that we can change the key order #9159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

specify how many segments were pruned by each server segment pruner#8884

specify how many segments were pruned by each server segment pruner#8884
walterddr merged 15 commits intoapache:masterfrom
gortiz:report-invalid-segments

gortiz commented Jun 13, 2022

Uh oh!

codecov-commenter commented Jun 14, 2022 •

edited

Loading

Uh oh!

xiangfu0 left a comment

Uh oh!

walterddr left a comment

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Uh oh!

gortiz commented Jun 22, 2022

Uh oh!

Jackie-Jiang Jun 23, 2022

Uh oh!

gortiz Jun 24, 2022

Uh oh!

Jackie-Jiang Jun 23, 2022 •

edited

Loading

Uh oh!

gortiz left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-      boolean isInvalid = isInvalidSegment(segment, query);
+      if (!isEmptySegment(segment)) {
+        if (isInvalidSegment(segment, query)) {
+          invalid++
+        } else {
+          segments.set(selected++, segment);
+        }
+      }

Conversation

gortiz commented Jun 13, 2022

Uh oh!

codecov-commenter commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

gortiz commented Jun 22, 2022

Uh oh!

Jackie-Jiang Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

gortiz Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gortiz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 14, 2022 •

edited

Loading

Jackie-Jiang Jun 23, 2022 •

edited

Loading

gortiz left a comment •

edited

Loading