-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Integrate enhanced SegmentProcessorFramework into MergeRollupTaskExecutor #7180
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
5ebd40b
to
abd83f6
Compare
assertEquals(segmentConfig.getMaxNumRecordsPerSegment(), SegmentConfig.DEFAULT_MAX_NUM_RECORDS_PER_SEGMENT); | ||
assertNull(segmentConfig.getSegmentNamePrefix()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
...src/main/java/org/apache/pinot/plugin/minion/tasks/merge_rollup/MergeRollupTaskExecutor.java
Show resolved
Hide resolved
...-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MergeTaskUtils.java
Show resolved
Hide resolved
...ion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MergeTaskUtilsTest.java
Show resolved
Hide resolved
There was a problem hiding this 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.
pinot-core/src/main/java/org/apache/pinot/core/segment/processing/framework/SegmentConfig.java
Show resolved
Hide resolved
...-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MergeTaskUtils.java
Show resolved
Hide resolved
LGTM, thanks for working on this! |
Wire the enhanced
SegmentProcessorFramework
into the merge/rollup task executor with the following support:Extract the common logic in
RealtimeToOfflineSegmentsTaskExecutor
andMergeRollupTaskExecutor