suneet-s commented on code in PR #16510:
URL: https://github.com/apache/druid/pull/16510#discussion_r1626939886
##########
docs/development/extensions-contrib/k8s-jobs.md:
##########
@@ -217,6 +217,66 @@ data:
druid.peon.mode=remote
druid.indexer.task.encapsulatedTask=true
```
+#### Dynamic Pod Template Selection Config
Review Comment:
note to self: doc should be re-written. remove use of `new feature`, `more
flexible`, etc.
What is the right point to talk about this config
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/common/KubernetesPeonClient.java:
##########
@@ -274,6 +279,14 @@ private void emitK8sPodMetrics(Task task, String metric,
long durationMs)
{
ServiceMetricEvent.Builder metricBuilder = new
ServiceMetricEvent.Builder();
IndexTaskUtils.setTaskDimensions(metricBuilder, task);
+ ExecutionConfig executionConfig = executionConfigRef.get();
+ if (executionConfig != null && executionConfig.getBehaviorStrategy() !=
null) {
+ metricBuilder.setDimensionIfNotNull(
+ "category",
+ executionConfig.getBehaviorStrategy().getTaskCategory(task)
+ );
+ }
Review Comment:
This seems incorrect. The executionConfig could have changed from the time
the task was converted to a job to when executionConfig.getBehaviorStrategy()
is called in this function.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/DefaultExecutionConfig.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.base.Preconditions;
+
+import java.util.Objects;
+
+public class DefaultExecutionConfig implements ExecutionConfig
Review Comment:
rename to TaskTypeExecutionConfig to indicate what it is doing
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/DefaultExecutionBehaviorStrategy.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import org.apache.druid.indexing.common.task.Task;
+
+/**
+ * This strategy defines how tasks are categorized based on their type for
execution purposes.
+ *
+ * This implementation categorizes tasks by simply returning the type of the
task,
+ * making it a straightforward, type-based categorization strategy.
+ */
+public class DefaultExecutionBehaviorStrategy implements
ExecutionBehaviorStrategy
Review Comment:
Rename to `TaskTypeExecutionBehaviorStrategy` instead of `Default` to be
more descriptive of what this class is trying to do.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/KubernetesResource.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.google.common.collect.ImmutableMap;
+import com.sun.jersey.spi.container.ResourceFilters;
+import org.apache.druid.audit.AuditEntry;
+import org.apache.druid.audit.AuditManager;
+import org.apache.druid.common.config.ConfigManager;
+import org.apache.druid.common.config.JacksonConfigManager;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.http.security.ConfigResourceFilter;
+import org.apache.druid.server.security.AuthorizationUtils;
+import org.joda.time.Interval;
+
+import javax.inject.Inject;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Resource that manages Kubernetes-specific execution configurations for
running tasks.
+ *
+ * <p>This class handles the CRUD operations for execution configurations and
provides
+ * endpoints to update, retrieve, and manage the history of these
configurations.</p>
+ */
+@Path("/druid/indexer/v1/k8s/runner")
+public class KubernetesResource
Review Comment:
```suggestion
@Path("/druid/indexer/v1/k8s/taskRunner")
public class KubernetesTaskRunnerResource
```
OR
```suggestion
@Path("/druid/indexer/v1/k8s/taskRunner/executionConfig")
public class KubernetesTaskRunnerExecutionConfigResource
```
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/KubernetesResource.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.google.common.collect.ImmutableMap;
+import com.sun.jersey.spi.container.ResourceFilters;
+import org.apache.druid.audit.AuditEntry;
+import org.apache.druid.audit.AuditManager;
+import org.apache.druid.common.config.ConfigManager;
+import org.apache.druid.common.config.JacksonConfigManager;
+import org.apache.druid.java.util.common.Intervals;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.http.security.ConfigResourceFilter;
+import org.apache.druid.server.security.AuthorizationUtils;
+import org.joda.time.Interval;
+
+import javax.inject.Inject;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.Context;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Resource that manages Kubernetes-specific execution configurations for
running tasks.
+ *
+ * <p>This class handles the CRUD operations for execution configurations and
provides
+ * endpoints to update, retrieve, and manage the history of these
configurations.</p>
+ */
+@Path("/druid/indexer/v1/k8s/runner")
+public class KubernetesResource
+{
+ private static final Logger log = new Logger(KubernetesResource.class);
+ private final JacksonConfigManager configManager;
+ private final AuditManager auditManager;
+ private AtomicReference<ExecutionConfig> executionConfigRef = null;
+
+ @Inject
+ public KubernetesResource(
+ final JacksonConfigManager configManager,
+ final AuditManager auditManager
+ )
+ {
+ this.configManager = configManager;
+ this.auditManager = auditManager;
+ }
+
+ /**
+ * Updates the Kubernetes execution configuration.
+ *
+ * @param executionConfig the new execution configuration to set
+ * @param req the HTTP servlet request providing context for
audit information
+ * @return a response indicating the success or failure of the update
operation
+ */
+ @POST
+ @Path("/execution")
Review Comment:
```suggestion
@Path("/executionConfig")
```
Similar comment for other APIs
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/ExecutionConfig.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+
+/**
+ * Represents the configuration for task execution within a Kubernetes
environment.
+ * This interface allows for dynamic configuration of task execution
strategies based
+ * on specified behavior strategies.
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl =
DefaultExecutionConfig.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "default", value = DefaultExecutionConfig.class)
+})
+public interface ExecutionConfig
+{
+ String CONFIG_KEY = "k8s.taskrunner.config";
Review Comment:
I too was confused about this name. I think having a more narrowly scoped
interface will be easier to understand and maintain.
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/execution/ExecutionBehaviorStrategy.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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.druid.k8s.overlord.execution;
+
+import com.fasterxml.jackson.annotation.JsonSubTypes;
+import com.fasterxml.jackson.annotation.JsonTypeInfo;
+import org.apache.druid.indexing.common.task.Task;
+
+import javax.annotation.Nullable;
+
+/**
+ * Defines a strategy for determining the execution behavior of tasks based on
specific conditions.
+ */
+@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl =
DefaultExecutionBehaviorStrategy.class)
+@JsonSubTypes(value = {
+ @JsonSubTypes.Type(name = "default", value =
DefaultExecutionBehaviorStrategy.class),
+ @JsonSubTypes.Type(name = "dynamicTask", value =
DynamicTaskExecutionBehaviorStrategy.class),
+})
+public interface ExecutionBehaviorStrategy
Review Comment:
I don't understand the name of this interface. What is the execution
behavior strategy? It looks like this is just getting the name of a category
from a Task
##########
docs/development/extensions-contrib/k8s-jobs.md:
##########
@@ -217,6 +217,66 @@ data:
druid.peon.mode=remote
druid.indexer.task.encapsulatedTask=true
```
+#### Dynamic Pod Template Selection Config
+The Dynamic Pod Template Selection feature enhances the K8s extension by
enabling more flexible and dynamic selection of pod templates based on task
properties. This process is governed by the `ExecutionBehaviorStrategy`. Below
are the two strategies implemented:
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`DefaultExecutionBehaviorStrategy`| This strategy categorizes tasks based on
their type for execution purposes, implementing the existing behavior that maps
pod templates according to task type. | true |
+|`DynamicTaskExecutionBehaviorStrategy`| This strategy dynamically evaluates a
series of selectors, with each selector corresponding to a potential task
category.| false |
+
+`DynamicTaskExecutionBehaviorStrategy`, the strategy implementing this new
feature, is based on conditional selectors `categorySelectors` that match
against task properties. These selectors are ordered in the dynamic
configuration, with the first selector given the highest priority during the
evaluation process. This means that the selection process uses these ordered
conditions to determine a task’s category based on context tags and task
fields. The first matching condition immediately determines the category,
thereby prioritizing certain configurations over others. Once a category is
identified, it is used to map to different Peon Pod templates, enabling
tailored resource allocation and management that aligns with the specific
requirements of each task.
+
+Example Configuration:
+
+We define two categories in the configuration—`low-throughput` and
`medium-throughput`—each associated with specific task conditions and arranged
in a priority order.
+
+- Low Throughput Category: This is the first category evaluated and has the
highest priority. Tasks that have a context tag
`billingCategory=streaming_ingestion` and a datasource of `wikipedia` will be
classified under the `low-throughput` category. This classification directs
such tasks to utilize a predefined pod template optimized for low throughput
requirements.
+
+- Medium Throughput Category: If a task does not meet the low-throughput
criteria, the system will then evaluate it against the next selector in order.
In this example, if the task type is index_kafka, it will fall into the
`medium-throughput` category.
+```
+{
+ "type": "default",
+ "behaviorStrategy": {
+ "type": "dynamicTask",
+ "categorySelectors": [
+ {
+ "selectionKey": "low-throughput",
Review Comment:
```suggestion
"template": "low-throughput",
```
I think the selectionKey should be called template - since the runtime
properties and the code in `PodTemplateTaskAdapter` refers to them as templates
##########
extensions-contrib/kubernetes-overlord-extensions/src/main/java/org/apache/druid/k8s/overlord/KubernetesOverlordModule.java:
##########
@@ -98,6 +103,8 @@ public void configure(Binder binder)
.toProvider(RunnerStrategyProvider.class)
.in(LazySingleton.class);
configureTaskLogs(binder);
+
+ Jerseys.addResource(binder, KubernetesResource.class);
Review Comment:
KubernetesResource does not indicate what the resource is actually for.
Suggested rename `KubernetesTaskExecutionConfigResource`
--
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]
