[GitHub] [dolphinscheduler] zhongjiajie commented on a diff in pull request #10373: [Feature-6758][Task] Add limit resource usage for tasks base on cgroup

[GitBox <gi...@apache.org>]

zhongjiajie commented on code in PR #10373:
URL: https://github.com/apache/dolphinscheduler/pull/10373#discussion_r891917581


##########
docs/docs/en/guide/task/datax.md:
##########
@@ -19,6 +19,8 @@ DataX task type for executing DataX programs. For DataX nodes, the worker will e
 - **Environment Name**: Configure the environment name in which to run the script.
 - **Number of failed retry attempts**: The number of times the task failed to be resubmitted.
 - **Failed retry interval**: The time, in cents, interval for resubmitting the task after a failed task.
+- **Cpu quota**: Assign the specified CPU time quota to the task executed. Takes a percentage value. Default -1 means unlimited. For example, the full CPU load of one core is 100%,and that of 16 cores is 1600%.

Review Comment:
   Thanks for given an  example about how to config it



##########
dolphinscheduler-e2e/dolphinscheduler-e2e-case/src/test/resources/docker/file-manage/common.properties:
##########
@@ -85,3 +85,6 @@ aws.access.key.id=accessKey123
 aws.secret.access.key=secretKey123
 aws.region=us-east-1
 aws.endpoint=http://s3:9000
+
+# Task resource limit state
+task.resource.limit.state=false

Review Comment:
   we set false by default, and it will not available when user set any number of CPU and mem in web UI, should we set `true` as default?



##########
dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/entity/TaskInstance.java:
##########
@@ -753,4 +763,20 @@ public int getTaskGroupPriority() {
     public void setTaskGroupPriority(int taskGroupPriority) {
         this.taskGroupPriority = taskGroupPriority;
     }
+
+    public Integer getCpuQuota() {
+        return cpuQuota;
+    }
+
+    public void setCpuQuota(Integer cpuQuota) {
+        this.cpuQuota = cpuQuota;

Review Comment:
   I have a question here, should we handle the `null` value in all set CPU and mem place to keep consistency. I know taskdefinitionlog and task instance come from taskdefintion, but I think keep consistency is not bad for all cases. But I am not have strong opinion here



##########
dolphinscheduler-dao/src/main/resources/sql/dolphinscheduler_h2.sql:
##########
@@ -486,6 +486,8 @@ CREATE TABLE t_ds_task_definition
     delay_time              int(11) DEFAULT '0',
     task_group_id           int(11) DEFAULT NULL,
     task_group_priority     tinyint(4) DEFAULT '0',
+    cpu_quota               int(11) DEFAULT '-1' NOT NULL,
+    memory_max              int(11) DEFAULT '-1' NOT NULL,

Review Comment:
   I think we should discuss about thoes column adding. Out table column is too many IMO, and this property is only use by some of the tasks not all of them, do we really need to add them into separate columns? should we add some column name `extra` or some name else, and put those two columns and others extra column only for some of the tasks in the further to keep our column as less column as possible?



-- 
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@dolphinscheduler.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org