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

Integrate enhanced SegmentProcessorFramework into MergeRollupTaskExecutor #7180

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

Jackie-Jiang
Copy link
Contributor

Wire the enhanced SegmentProcessorFramework into the merge/rollup task executor with the following support:

  • Concat/rollup/dedup
  • Null values
  • Custom segment name prefix

Extract the common logic in RealtimeToOfflineSegmentsTaskExecutor and MergeRollupTaskExecutor

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021

Codecov Report

Merging #7180 (936f734) into master (fe83e95) will increase coverage by 0.04%.
The diff coverage is 86.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7180      +/-   ##
============================================
+ Coverage     73.51%   73.56%   +0.04%     
  Complexity       92       92              
============================================
  Files          1506     1506              
  Lines         73832    73807      -25     
  Branches      10655    10650       -5     
============================================
+ Hits          54281    54293      +12     
+ Misses        16011    15976      -35     
+ Partials       3540     3538       -2     
Flag Coverage Δ
integration 41.95% <40.44%> (+0.02%) ⬆️
unittests 65.22% <83.14%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
.../org/apache/pinot/core/common/MinionConstants.java 0.00% <0.00%> (ø)
...re/segment/processing/framework/SegmentConfig.java 87.50% <ø> (ø)
...on/tasks/merge_rollup/MergeRollupTaskExecutor.java 88.63% <61.53%> (+6.41%) ⬆️
...egments/RealtimeToOfflineSegmentsTaskExecutor.java 94.20% <85.71%> (+1.41%) ⬆️
...ache/pinot/plugin/minion/tasks/MergeTaskUtils.java 96.00% <96.00%> (ø)
...inion/tasks/merge_rollup/MergeRollupTaskUtils.java 90.90% <100.00%> (+3.40%) ⬆️
...va/org/apache/pinot/common/minion/Granularity.java 0.00% <0.00%> (-100.00%) ⬇️
.../helix/core/minion/MinionInstancesCleanupTask.java 60.86% <0.00%> (-8.70%) ⬇️
...nction/DistinctCountBitmapAggregationFunction.java 46.90% <0.00%> (-8.25%) ⬇️
...mpl/dictionary/DoubleOffHeapMutableDictionary.java 57.44% <0.00%> (-5.32%) ⬇️
... and 24 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 fe83e95...936f734. Read the comment docs.

@Jackie-Jiang Jackie-Jiang force-pushed the merge_rollup_executor branch from 5ebd40b to abd83f6 Compare July 20, 2021 05:31
@Jackie-Jiang Jackie-Jiang requested a review from snleee July 20, 2021 05:32
assertEquals(segmentConfig.getMaxNumRecordsPerSegment(), SegmentConfig.DEFAULT_MAX_NUM_RECORDS_PER_SEGMENT);
assertNull(segmentConfig.getSegmentNamePrefix());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jtao15 Please review this PR since you need to rebase this for your scheduler PR.

@Jackie-Jiang Jackie-Jiang merged commit e0163f4 into apache:master Jul 21, 2021
@Jackie-Jiang Jackie-Jiang deleted the merge_rollup_executor branch July 21, 2021 03:26
@jtao15
Copy link
Contributor

jtao15 commented Jul 21, 2021

LGTM, thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants