snleee commented on a change in pull request #7178:
URL: https://github.com/apache/pinot/pull/7178#discussion_r684502706
##########
File path:
pinot-tools/src/main/java/org/apache/pinot/tools/BootstrapTableTool.java
##########
@@ -31,7 +31,7 @@
import java.util.Map;
import org.apache.commons.io.FileUtils;
import org.apache.pinot.common.minion.MinionClient;
-import org.apache.pinot.core.common.MinionConstants;
+import org.apache.pinot.common.minion.MinionConstants;
Review comment:
This change will incur backward incompatibility if the library pulls
`MinionConstants`. Can you tag the `release-note` + `backward incompatibility`
for this PR?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
##########
@@ -297,10 +300,15 @@ public static void validateIngestionConfig(TableConfig
tableConfig, @Nullable Sc
}
}
- private static void validateTaskConfigs(TableConfig tableConfig) {
+ private static void validateTaskConfigs(TableConfig tableConfig, Schema
schema) {
Review comment:
Can we add some unit test for the task config validation for
merge/rollup?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
##########
@@ -313,6 +321,65 @@ private static void validateTaskConfigs(TableConfig
tableConfig) {
}
}
+ private static void validateMergeRollupTaskConfig(TableTaskConfig
tableTaskConfig, Schema schema) {
+ if (tableTaskConfig != null &&
tableTaskConfig.isTaskTypeEnabled(MinionConstants.MergeRollupTask.TASK_TYPE)) {
+ Map<String, String> taskTypeConfig =
tableTaskConfig.getConfigsForTaskType(MinionConstants.MergeRollupTask.TASK_TYPE);
+ Map<String, Map<String, String>> levelToConfigMap =
MergeRollupTaskUtils.getLevelToConfigMap(taskTypeConfig);
+ boolean validateAggregateFunction = false;
+ for (Map.Entry<String, Map<String, String>> levelToConfigEntry :
levelToConfigMap.entrySet()) {
+ String mergeLevel = levelToConfigEntry.getKey();
+ Map<String, String> mergeConfig = levelToConfigEntry.getValue();
+ String mergeType =
mergeConfig.get(MinionConstants.MergeRollupTask.MERGE_TYPE_KEY);
+ Preconditions.checkState(
+ mergeType != null && (mergeType.equalsIgnoreCase("CONCAT") ||
mergeType.equalsIgnoreCase("ROLLUP")),
+ "Must provide valid merge type: 'CONCAT' or 'ROLLUP' for merge
level: %s", mergeLevel);
+ String bucketTimePeriod =
mergeConfig.get(MinionConstants.MergeRollupTask.BUCKET_TIME_PERIOD_KEY);
+ Preconditions.checkState(isValidateTimePeriod(bucketTimePeriod),
+ "Must provide valid 'bucketTimePeriod' for merge level: %s",
mergeLevel);
+ String bufferTimePeriod =
mergeConfig.get(MinionConstants.MergeRollupTask.BUFFER_TIME_PERIOD_KEY);
+ Preconditions.checkState(isValidateTimePeriod(bufferTimePeriod),
+ "Must provide valid 'bufferTimePeriod' for merge level: %s",
mergeLevel);
+ if (mergeType.equalsIgnoreCase("ROLLUP")) {
Review comment:
type == MergeType.ROLLUP
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
##########
@@ -313,6 +321,65 @@ private static void validateTaskConfigs(TableConfig
tableConfig) {
}
}
+ private static void validateMergeRollupTaskConfig(TableTaskConfig
tableTaskConfig, Schema schema) {
+ if (tableTaskConfig != null &&
tableTaskConfig.isTaskTypeEnabled(MinionConstants.MergeRollupTask.TASK_TYPE)) {
+ Map<String, String> taskTypeConfig =
tableTaskConfig.getConfigsForTaskType(MinionConstants.MergeRollupTask.TASK_TYPE);
+ Map<String, Map<String, String>> levelToConfigMap =
MergeRollupTaskUtils.getLevelToConfigMap(taskTypeConfig);
+ boolean validateAggregateFunction = false;
+ for (Map.Entry<String, Map<String, String>> levelToConfigEntry :
levelToConfigMap.entrySet()) {
+ String mergeLevel = levelToConfigEntry.getKey();
+ Map<String, String> mergeConfig = levelToConfigEntry.getValue();
+ String mergeType =
mergeConfig.get(MinionConstants.MergeRollupTask.MERGE_TYPE_KEY);
+ Preconditions.checkState(
+ mergeType != null && (mergeType.equalsIgnoreCase("CONCAT") ||
mergeType.equalsIgnoreCase("ROLLUP")),
Review comment:
Instead of hard-coding `CONCAT, ROLLUP`, I think that the following it
better:
```
MergeType type;
try {
type = MergeType.valueOf(mergeType.toUpperCase())
} catch (IllegalArgumentException e) {
// fail
}
```
Otherwise, we need to add here when we add the new merge type.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
