sajjad-moradi commented on code in PR #12092:
URL: https://github.com/apache/pinot/pull/12092#discussion_r1631561479
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java:
##########
@@ -345,20 +346,24 @@ public List<PinotTaskConfig>
generateTasks(List<TableConfig> tableConfigs) {
// Haven't find the first overlapping segment, continue to the
next segment
} else {
// Has gone through all overlapping segments for current bucket
- if (hasUnmergedSegments && areAllSegmentsReadyToMerge) {
+ if (hasUnmergedSegments && isAllSegmentsReadyToMerge) {
// Add the bucket if there are unmerged segments
selectedSegmentsForAllBuckets.add(selectedSegmentsForBucket);
}
- if (selectedSegmentsForAllBuckets.size() == maxNumParallelBuckets
|| hasSpilledOverData) {
+ if (selectedSegmentsForAllBuckets.size() == maxNumParallelBuckets
+ || hasSpilledOverData && hasUnmergedSegments &&
isAllSegmentsReadyToMerge) {
// If there are enough buckets or found spilled over data,
schedule merge tasks
+ // Note: the check for hasUnmergedSegments &&
isAllSegmentsReadyToMerge is needed for processAll mode.
+ // This check prevents a scenario where lower-level merge
discontinues scheduling due to the higher-level
+ // segments being considered as spilled-over data based on
lower-level criteria.
Review Comment:
Do we have a unit test covering the newly added boolean terms?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
