You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "rajagopr (via GitHub)" <gi...@apache.org> on 2024/10/30 20:47:08 UTC
[PR] Added support to perform task validations for plug-in tasks [pinot]
rajagopr opened a new pull request, #14340:
URL: https://github.com/apache/pinot/pull/14340
## Description
Added support to perform task specific validations for the in-built Pinot tasks as well as the plug-in tasks. Modified the `PinotTaskGenerator` interface such that each task can implement it's own custom validation.
## Notes
The new utils class `TaskConfigUtils` is kept within the `pinot-controller` module as the existing `TableConfigUtils` resides within the module `pinot-segment-local` and this package doesn't take a dependency on the `pinot-controller` module. The `PinotTaskGenerator` lies within the `pinot-controller` which is required to delegate the task level validation. `pinot-segment-local` depending on `pinot-controller` would create cyclic dependency.
## Testing
Added unit tests and tested locally.
**Add table configs**
```
2024/10/30 13:07:10.212 WARN [TaskConfigUtils] [grizzly-http-server-3] Task generator not found for task type: MyCustomTask, while validating table configs for table: students001_OFFLINE
```
**Validate table configs**
```
2024/10/30 13:13:56.423 WARN [TaskConfigUtils] [grizzly-http-server-2] Task generator not found for task type: MyCustomTask, while validating table configs for table: students001_OFFLINE
```
**Update table configs**
```
2024/10/30 13:21:18.566 WARN [TaskConfigUtils] [grizzly-http-server-0] Task generator not found for task type: MyCustomTask2, while validating table configs for table: students001_OFFLINE
```
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1827227693
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
Review Comment:
Why not move this util to `PinotTaskManager` itself?
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "rajagopr (via GitHub)" <gi...@apache.org>.
rajagopr commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1828165485
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
+ if (tableConfig == null || tableConfig.getTaskConfig() == null) {
+ return;
+ }
+ TableTaskConfig taskConfig = tableConfig.getTaskConfig();
+ for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfigsMap().entrySet()) {
+ String taskType = taskConfigEntry.getKey();
+ TaskGeneratorRegistry taskRegistry = pinotTaskManager.getTaskGeneratorRegistry();
+ if (taskRegistry != null) {
+ PinotTaskGenerator taskGenerator = taskRegistry.getTaskGenerator(taskType);
+ if (taskGenerator != null) {
+ taskGenerator.validateTaskConfigs(tableConfig);
+ } else {
+ LOGGER.warn("Task generator not found for task type: {}, while validating table configs for table: {}",
Review Comment:
This is more of a safety check and ideally should not happen. However, it can happen under the following scenarios:
1) Required pinot task plugin is missing from the deployment. As a result, the relevant task is not registered.
2) In the task config, the user makes a typo.
It won't be validated anywhere as tasks are registered dynamically based on reflection.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "rajagopr (via GitHub)" <gi...@apache.org>.
rajagopr commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1828421138
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
Review Comment:
Yes, I will post a new revision in which the task specific validations would be moved to the relevant task generators.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "rajagopr (via GitHub)" <gi...@apache.org>.
rajagopr commented on PR #14340:
URL: https://github.com/apache/pinot/pull/14340#issuecomment-2460437602
> LGTM otherwise.
@tibrewalpratik17 – Could you approve as well if no more concerns?
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish merged PR #14340:
URL: https://github.com/apache/pinot/pull/14340
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1829245519
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
+ if (tableConfig == null || tableConfig.getTaskConfig() == null) {
+ return;
+ }
+ TableTaskConfig taskConfig = tableConfig.getTaskConfig();
+ for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfigsMap().entrySet()) {
+ String taskType = taskConfigEntry.getKey();
+ TaskGeneratorRegistry taskRegistry = pinotTaskManager.getTaskGeneratorRegistry();
+ if (taskRegistry != null) {
+ PinotTaskGenerator taskGenerator = taskRegistry.getTaskGenerator(taskType);
+ if (taskGenerator != null) {
+ taskGenerator.validateTaskConfigs(tableConfig);
+ } else {
+ LOGGER.warn("Task generator not found for task type: {}, while validating table configs for table: {}",
Review Comment:
Shouldn't we throw an exception instead of warning ?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/minion/generator/PinotTaskGenerator.java:
##########
@@ -99,4 +99,10 @@ default void nonLeaderCleanUp(List<String> tableNamesWithType) {
default String getMinionInstanceTag(TableConfig tableConfig) {
return CommonConstants.Helix.UNTAGGED_MINION_INSTANCE;
}
+
+ /**
+ * Performs necessary validations for the given task type.
+ */
+ default void validateTaskConfigs(TableConfig tableConfig, Map<String, String> taskConfigs) {
Review Comment:
May be add doc or rename the param to indicate that taskConfigs here is the input taskConfigs that needs to be validated.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1832443944
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskGenerator.java:
##########
@@ -304,4 +304,48 @@ static boolean validate(TableConfig tableConfig) {
}
return true;
}
+
+ @Override
+ public void validateTaskConfigs(TableConfig tableConfig, Map<String, String> taskConfigs) {
+ // check table is realtime
+ Preconditions.checkState(tableConfig.getTableType() == TableType.REALTIME,
+ "UpsertCompactionTask only supports realtime tables!");
+ // check upsert enabled
+ Preconditions.checkState(tableConfig.isUpsertEnabled(), "Upsert must be enabled for UpsertCompactionTask");
+
+ // check no malformed period
+ if (taskConfigs.containsKey("bufferTimePeriod")) {
+ TimeUtils.convertPeriodToMillis(taskConfigs.get("bufferTimePeriod"));
+ }
+ // check invalidRecordsThresholdPercent
+ if (taskConfigs.containsKey("invalidRecordsThresholdPercent")) {
+ Preconditions.checkState(Double.parseDouble(taskConfigs.get("invalidRecordsThresholdPercent")) >= 0
+ && Double.parseDouble(taskConfigs.get("invalidRecordsThresholdPercent")) <= 100,
+ "invalidRecordsThresholdPercent must be >= 0 and <= 100");
+ }
+ // check invalidRecordsThresholdCount
+ if (taskConfigs.containsKey("invalidRecordsThresholdCount")) {
+ Preconditions.checkState(Long.parseLong(taskConfigs.get("invalidRecordsThresholdCount")) >= 1,
+ "invalidRecordsThresholdCount must be >= 1");
+ }
+ // check that either invalidRecordsThresholdPercent or invalidRecordsThresholdCount was provided
+ Preconditions.checkState(
+ taskConfigs.containsKey("invalidRecordsThresholdPercent") || taskConfigs.containsKey(
Review Comment:
let's use MinionConstants variables here instead of hardcoding the names.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1832117993
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.EnumSet;
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.quartz.CronScheduleBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.pinot.segment.spi.AggregationFunctionType.*;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+ private static final String SCHEDULE_KEY = "schedule";
+ private static final String ALL_VALIDATION_TYPE = "ALL";
+ private static final String TASK_VALIDATION_TYPE = "TASK";
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager,
+ String validationTypesToSkip) {
+ if (tableConfig == null || tableConfig.getTaskConfig() == null) {
+ return;
+ }
+ if (ALL_VALIDATION_TYPE.equalsIgnoreCase(validationTypesToSkip) || TASK_VALIDATION_TYPE.equalsIgnoreCase(
+ validationTypesToSkip)) {
+ LOGGER.info("Skipping task validation as validationTypesToSkip is set to: {}", validationTypesToSkip);
+ return;
+ }
+ TableTaskConfig taskConfig = tableConfig.getTaskConfig();
+ for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfigsMap().entrySet()) {
+ String taskType = taskConfigEntry.getKey();
+ TaskGeneratorRegistry taskRegistry = pinotTaskManager.getTaskGeneratorRegistry();
+ if (taskRegistry != null) {
+ PinotTaskGenerator taskGenerator = taskRegistry.getTaskGenerator(taskType);
+ if (taskGenerator != null) {
+ Map<String, String> taskConfigs = taskConfigEntry.getValue();
+ doCommonTaskValidations(tableConfig, taskType, taskConfigs);
+ taskGenerator.validateTaskConfigs(tableConfig, taskConfigs);
+ } else {
+ throw new RuntimeException(String.format("Task generator not found for task type: %s, while validating table "
+ + "configs for table: %s", taskType, tableConfig.getTableName()));
+ }
+ }
+ }
+ }
+
+ @VisibleForTesting
+ static void doCommonTaskValidations(TableConfig tableConfig, String taskType,
+ Map<String, String> taskConfigs) {
+ Preconditions.checkNotNull(taskConfigs, String.format("Task configuration for %s cannot be null.", taskType));
+ // Schedule key for task config has to be set.
+ if (taskConfigs.containsKey(SCHEDULE_KEY)) {
+ String cronExprStr = taskConfigs.get(SCHEDULE_KEY);
+ try {
+ CronScheduleBuilder.cronSchedule(cronExprStr);
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ String.format("Task %s contains an invalid cron schedule: %s", taskType, cronExprStr), e);
+ }
+ }
+ boolean isAllowDownloadFromServer = Boolean.parseBoolean(
+ taskConfigs.getOrDefault(TableTaskConfig.MINION_ALLOW_DOWNLOAD_FROM_SERVER,
+ String.valueOf(TableTaskConfig.DEFAULT_MINION_ALLOW_DOWNLOAD_FROM_SERVER)));
+ if (isAllowDownloadFromServer) {
+ Preconditions.checkState(tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() != null,
+ String.format("Table %s has task %s with allowDownloadFromServer set to true, but "
+ + "peerSegmentDownloadScheme is not set in the table config", tableConfig.getTableName(),
+ taskType));
+ }
+ }
+
+ public final static EnumSet<AggregationFunctionType> AVAILABLE_CORE_VALUE_AGGREGATORS =
Review Comment:
Can we move this to MinionConstants class instead.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1827450353
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
Review Comment:
> Are we creating a generator every time? I think its a factory instance created once and used there after.
Yeah my bad! Just checked we are using the same generator.
> The motivation for doing this is that we want to provide the ability for custom task implementations to add their validations. This is to fail table updates, if there are incompatible or incorrect configs in the taskConfig of a particular task.
Sounds good to me! I guess it's okay to fail during table-config update for better visibility rather than silently failing tasks during generation process.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1832117993
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.util.EnumSet;
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.segment.spi.AggregationFunctionType;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.quartz.CronScheduleBuilder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.pinot.segment.spi.AggregationFunctionType.*;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+ private static final String SCHEDULE_KEY = "schedule";
+ private static final String ALL_VALIDATION_TYPE = "ALL";
+ private static final String TASK_VALIDATION_TYPE = "TASK";
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager,
+ String validationTypesToSkip) {
+ if (tableConfig == null || tableConfig.getTaskConfig() == null) {
+ return;
+ }
+ if (ALL_VALIDATION_TYPE.equalsIgnoreCase(validationTypesToSkip) || TASK_VALIDATION_TYPE.equalsIgnoreCase(
+ validationTypesToSkip)) {
+ LOGGER.info("Skipping task validation as validationTypesToSkip is set to: {}", validationTypesToSkip);
+ return;
+ }
+ TableTaskConfig taskConfig = tableConfig.getTaskConfig();
+ for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfigsMap().entrySet()) {
+ String taskType = taskConfigEntry.getKey();
+ TaskGeneratorRegistry taskRegistry = pinotTaskManager.getTaskGeneratorRegistry();
+ if (taskRegistry != null) {
+ PinotTaskGenerator taskGenerator = taskRegistry.getTaskGenerator(taskType);
+ if (taskGenerator != null) {
+ Map<String, String> taskConfigs = taskConfigEntry.getValue();
+ doCommonTaskValidations(tableConfig, taskType, taskConfigs);
+ taskGenerator.validateTaskConfigs(tableConfig, taskConfigs);
+ } else {
+ throw new RuntimeException(String.format("Task generator not found for task type: %s, while validating table "
+ + "configs for table: %s", taskType, tableConfig.getTableName()));
+ }
+ }
+ }
+ }
+
+ @VisibleForTesting
+ static void doCommonTaskValidations(TableConfig tableConfig, String taskType,
+ Map<String, String> taskConfigs) {
+ Preconditions.checkNotNull(taskConfigs, String.format("Task configuration for %s cannot be null.", taskType));
+ // Schedule key for task config has to be set.
+ if (taskConfigs.containsKey(SCHEDULE_KEY)) {
+ String cronExprStr = taskConfigs.get(SCHEDULE_KEY);
+ try {
+ CronScheduleBuilder.cronSchedule(cronExprStr);
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ String.format("Task %s contains an invalid cron schedule: %s", taskType, cronExprStr), e);
+ }
+ }
+ boolean isAllowDownloadFromServer = Boolean.parseBoolean(
+ taskConfigs.getOrDefault(TableTaskConfig.MINION_ALLOW_DOWNLOAD_FROM_SERVER,
+ String.valueOf(TableTaskConfig.DEFAULT_MINION_ALLOW_DOWNLOAD_FROM_SERVER)));
+ if (isAllowDownloadFromServer) {
+ Preconditions.checkState(tableConfig.getValidationConfig().getPeerSegmentDownloadScheme() != null,
+ String.format("Table %s has task %s with allowDownloadFromServer set to true, but "
+ + "peerSegmentDownloadScheme is not set in the table config", tableConfig.getTableName(),
+ taskType));
+ }
+ }
+
+ public final static EnumSet<AggregationFunctionType> AVAILABLE_CORE_VALUE_AGGREGATORS =
Review Comment:
Let's move this to MinionConstants class instead.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1827433650
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
Review Comment:
@shounakmk219 , @tibrewalpratik17
The motivation for doing this is that we want to provide the ability for custom task implementations to add their validations. This is to fail table updates, if there are incompatible or incorrect configs in the taskConfig of a particular task. Example - Instead of doing the below, we would have the corresponding realtime to offline task generator contain the validation. All task specific validations would happen in the corresponding generators.
// Task Specific validation for REALTIME_TO_OFFLINE_TASK_TYPE
// TODO task specific validate logic should directly call to PinotTaskGenerator API
if (taskTypeConfigName.equals(REALTIME_TO_OFFLINE_TASK_TYPE)) {
// check table is not upsert
Are we creating a generator every time? I think its a factory instance created once and used there after.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1826976907
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
+ if (tableConfig == null || tableConfig.getTaskConfig() == null) {
+ return;
+ }
+ TableTaskConfig taskConfig = tableConfig.getTaskConfig();
+ for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfigsMap().entrySet()) {
+ String taskType = taskConfigEntry.getKey();
+ TaskGeneratorRegistry taskRegistry = pinotTaskManager.getTaskGeneratorRegistry();
+ if (taskRegistry != null) {
+ PinotTaskGenerator taskGenerator = taskRegistry.getTaskGenerator(taskType);
+ if (taskGenerator != null) {
+ taskGenerator.validateTaskConfigs(tableConfig);
+ } else {
+ LOGGER.warn("Task generator not found for task type: {}, while validating table configs for table: {}",
Review Comment:
This is actually a wrong config? Probably validated elsewhere, if not we can do here itself. No generator for taskType.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
Review Comment:
Dont we want to consolidate the already existing one as well here? This method can call the individual generator's validation function, when it iterates over task config map.
@VisibleForTesting
static void validateTaskConfigs(TableConfig tableConfig, Schema schema) {
TableTaskConfig taskConfig = tableConfig.getTaskConfig();
if (taskConfig != null) {
for (Map.Entry<String, Map<String, String>> taskConfigEntry : taskConfig.getTaskTypeConfig
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1827259440
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
Review Comment:
+1 to this! The only motivation of doing it as a separate class is to have validations during updating / creating table config. Moving to PinotTaskManager can help validate configs during task-scheduling runs in controller.
With the present implementation, we end up creating a generator class everytime table config is modified which doesn't seem right.
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14340:
URL: https://github.com/apache/pinot/pull/14340#issuecomment-2448421164
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14340?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`008e09b`)](https://app.codecov.io/gh/apache/pinot/commit/008e09befb99cc7615c2b2929433dcfe4f7b63f1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1265 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (008e09b). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (008e09b) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|0|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|1|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14340 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 3 -2433
Lines 133233 6 -133227
Branches 20636 0 -20636
=============================================
- Hits 82274 0 -82274
+ Misses 44911 6 -44905
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.71%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14340/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14340?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Added support to perform task validations for plug-in tasks [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #14340:
URL: https://github.com/apache/pinot/pull/14340#discussion_r1827433650
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/TaskConfigUtils.java:
##########
@@ -0,0 +1,56 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.controller.util;
+
+import java.util.Map;
+import org.apache.pinot.controller.helix.core.minion.PinotTaskManager;
+import org.apache.pinot.controller.helix.core.minion.generator.PinotTaskGenerator;
+import org.apache.pinot.controller.helix.core.minion.generator.TaskGeneratorRegistry;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class TaskConfigUtils {
+ private static final Logger LOGGER = LoggerFactory.getLogger(TaskConfigUtils.class);
+
+ private TaskConfigUtils() {
+ }
+
+ public static void validateTaskConfigs(TableConfig tableConfig, PinotTaskManager pinotTaskManager) {
Review Comment:
The motivation for doing this is that we want to provide the ability for custom task implementations to add their validations. This is to fail table updates, if there are incompatible or incorrect configs in the taskConfig of a particular task. Example - Instead of doing the below, we would have the corresponding realtime to offline task generator contain the validation. All task specific validations would happen in the corresponding generators.
// Task Specific validation for REALTIME_TO_OFFLINE_TASK_TYPE
// TODO task specific validate logic should directly call to PinotTaskGenerator API
if (taskTypeConfigName.equals(REALTIME_TO_OFFLINE_TASK_TYPE)) {
// check table is not upsert
Are we creating a generator every time? I think its a factory instance created once and used there after.
--
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