You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "jtao15 (via GitHub)" <gi...@apache.org> on 2023/07/27 16:27:51 UTC
[GitHub] [pinot] jtao15 commented on a diff in pull request #10979: Skip generating rollup task with all merged segments when "skipAllMerged" mode is enabled
jtao15 commented on code in PR #10979:
URL: https://github.com/apache/pinot/pull/10979#discussion_r1276536454
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java:
##########
@@ -663,18 +672,26 @@ private List<PinotTaskConfig> createPinotTaskConfigs(List<SegmentZKMetadata> sel
int numRecordsPerTask = 0;
List<List<String>> segmentNamesList = new ArrayList<>();
List<List<String>> downloadURLsList = new ArrayList<>();
+ List<SegmentZKMetadata> segmentZKMetadataList = new ArrayList<>();
List<String> segmentNames = new ArrayList<>();
List<String> downloadURLs = new ArrayList<>();
for (int i = 0; i < segments.size(); i++) {
SegmentZKMetadata targetSegment = segments.get(i);
+ segmentZKMetadataList.add(targetSegment);
segmentNames.add(targetSegment.getSegmentName());
downloadURLs.add(targetSegment.getDownloadUrl());
numRecordsPerTask += targetSegment.getTotalDocs();
if (numRecordsPerTask >= maxNumRecordsPerTask || i == segments.size() - 1) {
- segmentNamesList.add(segmentNames);
- downloadURLsList.add(downloadURLs);
+ // If "skipAllMerged" mode is enabled, skip generating task with all merged segments
+ boolean skipAllMerged = MergeTask.SKIP_ALL_MERGED_MODE.equalsIgnoreCase(taskConfigs.get(MergeTask.MODE));
Review Comment:
Considering that `processAll` and `skipAllMerged` are not mutually exclusive, a table can have multiple modes. One approach is to treat the task modes as a comma-separated list and then verify if `skipAllMerged` is enabled within that list.(nit) put this line outside of the for loop
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java:
##########
@@ -663,18 +672,26 @@ private List<PinotTaskConfig> createPinotTaskConfigs(List<SegmentZKMetadata> sel
int numRecordsPerTask = 0;
List<List<String>> segmentNamesList = new ArrayList<>();
List<List<String>> downloadURLsList = new ArrayList<>();
+ List<SegmentZKMetadata> segmentZKMetadataList = new ArrayList<>();
List<String> segmentNames = new ArrayList<>();
List<String> downloadURLs = new ArrayList<>();
for (int i = 0; i < segments.size(); i++) {
SegmentZKMetadata targetSegment = segments.get(i);
+ segmentZKMetadataList.add(targetSegment);
segmentNames.add(targetSegment.getSegmentName());
downloadURLs.add(targetSegment.getDownloadUrl());
numRecordsPerTask += targetSegment.getTotalDocs();
if (numRecordsPerTask >= maxNumRecordsPerTask || i == segments.size() - 1) {
- segmentNamesList.add(segmentNames);
- downloadURLsList.add(downloadURLs);
+ // If "skipAllMerged" mode is enabled, skip generating task with all merged segments
+ boolean skipAllMerged = MergeTask.SKIP_ALL_MERGED_MODE.equalsIgnoreCase(taskConfigs.get(MergeTask.MODE));
Review Comment:
Considering that `processAll` and `skipAllMerged` are not mutually exclusive, a table can have multiple modes. One approach is to treat the task modes as a comma-separated list and then verify if `skipAllMerged` is enabled within that list.
(nit) put this line outside of the for loop
--
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