jtao15 commented on a change in pull request #7368:
URL: https://github.com/apache/pinot/pull/7368#discussion_r717956178
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -92,13 +97,69 @@
public class MergeRollupTaskGenerator implements PinotTaskGenerator {
private static final int DEFAULT_MAX_NUM_RECORDS_PER_TASK = 50_000_000;
private static final String REFRESH = "REFRESH";
+ private static final String MERGE_ROLLUP_TASK_DELAY_IN_NUM_BUCKETS =
"mergeRollupTaskDelayInNumBuckets";
private static final Logger LOGGER =
LoggerFactory.getLogger(MergeRollupTaskGenerator.class);
-
Review comment:
Looks like we usually keep one empty line between static and non-static
fields, should we keep this line?
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -110,17 +171,23 @@ public String getTaskType() {
public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
String taskType = MergeRollupTask.TASK_TYPE;
List<PinotTaskConfig> pinotTaskConfigs = new ArrayList<>();
-
+ Set<String> candidateMergeTables = new HashSet<>();
Review comment:
(nit) private?
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -339,6 +405,18 @@ public String getTaskType() {
.info("Finished generating task configs for table: {} for task: {},
numTasks: {}", offlineTableName, taskType,
pinotTaskConfigsForTable.size());
}
+
+ // Reset watermarks for invalid tables
Review comment:
I'm a little bit confused why we need to clean up metrics for invalid
tables since we will not create metrics for these tables. Are we trying to
handle schema evolution here?
##########
File path:
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/mergerollup/MergeRollupTaskGenerator.java
##########
@@ -110,17 +171,23 @@ public String getTaskType() {
public List<PinotTaskConfig> generateTasks(List<TableConfig> tableConfigs) {
String taskType = MergeRollupTask.TASK_TYPE;
List<PinotTaskConfig> pinotTaskConfigs = new ArrayList<>();
-
+ Set<String> candidateMergeTables = new HashSet<>();
for (TableConfig tableConfig : tableConfigs) {
if (!validate(tableConfig, taskType)) {
continue;
}
String offlineTableName = tableConfig.getTableName();
LOGGER.info("Start generating task configs for table: {} for task: {}",
offlineTableName, taskType);
+ candidateMergeTables.add(offlineTableName);
// Get all segment metadata
List<SegmentZKMetadata> allSegments =
_clusterInfoAccessor.getSegmentsZKMetadata(offlineTableName);
+ // Reset the watermark time if no segment found
+ if (allSegments.isEmpty()) {
Review comment:
Same as `line 409`, for empty table we will not add the metric since the
watermark is never set, so we won't need to remove the metric?
--
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
