[Improvement-17928][API] Add worker group id to environment relation by qiuyanjun888 · Pull Request #18349 · apache/dolphinscheduler

qiuyanjun888 · 2026-06-14T13:05:02Z

Was this PR generated or assisted by AI?

YES. This pull request was assisted by Hermes Agent / OpenAI Codex for code changes, focused tests, and local verification. The scope and final submission were directed by the contributor.

Purpose of the pull request

Closes #17928.

This pull request adds a stable worker group id to EnvironmentWorkerGroupRelation while keeping the existing worker group name field for compatibility.

Brief change log

Add workerGroupId / worker_group_id to EnvironmentWorkerGroupRelation, MyBatis mapping, base schema SQL, and upgrade SQL.
Backfill existing worker_group_id values in MySQL/PostgreSQL upgrade SQL by joining the legacy worker group name to t_ds_worker_group.
Persist workerGroupId when creating or updating environment-worker-group relations.
Use worker_group_id when checking environment dependencies before deleting worker groups, with legacy-name fallback only for rows whose worker_group_id is NULL.
Add focused DAO/API tests for persistence, create/update flows, and worker group delete dependency checks.

Verify this pull request

This change added tests and can be verified as follows:

git diff --check
./mvnw -pl dolphinscheduler-dao,dolphinscheduler-api spotless:check -DskipTests -DskipITs -Djacoco.skip=true
./mvnw -pl dolphinscheduler-dao,dolphinscheduler-api test -Dtest=EnvironmentWorkerGroupRelationMapperTest,EnvironmentServiceTest,WorkerGroupServiceTest -DskipITs -Dspotless.check.skip=true -Dcheckstyle.skip=true -Djacoco.skip=true

The focused Maven test run passed with 24 tests, 0 failures, 0 errors, and 0 skipped.

Pull Request Notice

ruanwenjun

Need to upgrade the data at db

qiuyanjun888 · 2026-06-15T01:22:28Z

Thanks for the review. I have addressed this in the latest commit: the upgrade DDL now only adds the worker_group_id column, and the existing data backfill has been moved into the matching DML upgrade scripts for both MySQL and PostgreSQL.

SbloodyS · 2026-06-15T13:08:56Z

+ALTER TABLE `t_ds_environment_worker_group_relation`
+ADD COLUMN `worker_group_id` int(11) DEFAULT NULL COMMENT 'worker group id';


You should put it into 3.5.0

Addressed in 05ef512. I moved the worker_group_id upgrade scripts from 3.4.1_schema to 3.5.0_schema for both MySQL and PostgreSQL. The 3.4.1 upgrade scripts are no longer changed in this PR.

Addressed on the current branch: the MySQL worker_group_id upgrade DDL/DML is under sql/upgrade/3.5.0_schema, and the 3.4.1 MySQL DDL no longer contains this change.

ruanwenjun · 2026-06-15T14:25:30Z

+    private Integer getWorkerGroupId(String workerGroupName) {
+        List<WorkerGroup> workerGroups = workerGroupDao.queryWorkerGroupByName(workerGroupName);
+        if (CollectionUtils.isEmpty(workerGroups)) {
+            return null;
+        }
+        return workerGroups.get(0).getId();
+    }
+


Can workerGroupDao.queryWorkerGroupByName return list?

If the worker group is not exist, need to throw exception rather than return null.

We need to use batch query, rather than query single item at a for-each.

Addressed in 05ef512. EnvironmentServiceImpl now parses the worker groups once and loads them with workerGroupDao.queryWorkerGroupByNames(...). Missing worker groups now throw WORKER_GROUP_NOT_EXIST instead of returning null.

Addressed on the current branch: WorkerGroupDao/Mapper returns a list, EnvironmentServiceImpl performs a single queryWorkerGroupByNames batch lookup, maps results by name, and throws WORKER_GROUP_NOT_EXIST when any requested worker group is missing.

ruanwenjun · 2026-06-15T14:26:32Z

+                        .eq("worker_group_id", workerGroup.getId())
+                        .or(queryWrapper -> queryWrapper.isNull("worker_group_id")
+                                .eq("worker_group", workerGroup.getName())));


The worker group name shouldn't be deprecated.

Addressed in 05ef512. The worker group dependency check now checks both worker_group_id and worker_group name, so worker_group is still treated as valid data rather than deprecated fallback only.

Addressed on the current branch: workerGroup remains a non-deprecated compatibility field, and worker group deletion dependency checks match environment relations by worker_group_id or by worker_group name.

ruanwenjun · 2026-06-15T14:29:18Z

+  `worker_group_id` int(11) DEFAULT NULL COMMENT 'worker group id',
+  `worker_group` varchar(255) NOT NULL COMMENT 'worker group name',


Why worker_group is not null but worker_group_id can be null?

Addressed in 05ef512. The base schema now defines worker_group_id as NOT NULL. For upgrades, DDL adds it nullable first, DML backfills it from t_ds_worker_group, and then the column is changed to NOT NULL for MySQL and PostgreSQL.

SbloodyS · 2026-06-16T06:06:04Z

        where name = #{name}
    </select>
+    <select id="queryWorkerGroupByNames" resultType="org.apache.dolphinscheduler.dao.entity.WorkerGroup">
+        select *


You should avoid wild card select.

Fixed in f592c03. queryWorkerGroupByNames now selects explicit columns instead of select *.

Addressed on the current branch: queryWorkerGroupByNames uses an explicit column list instead of a wildcard select.

SbloodyS · 2026-06-16T06:06:52Z

+    /**
+     * worker group name
+     */


Suggested change

/**

* worker group name

*/

Fixed in f592c03. Removed the extra worker group name comment.

Addressed on the current branch: EnvironmentWorkerGroupRelation keeps workerGroup without a deprecated marker in that area.

SbloodyS · 2026-06-16T06:07:06Z

+    /**
+     * test query worker groups by names
+     */


Suggested change

/**

* test query worker groups by names

*/

Fixed in f592c03. Removed the redundant test method comment.

Addressed on the current branch: WorkerGroupMapperTest covers queryWorkerGroupByNames without the suggested extra line.

+                    relationMapper.insert(relation);
+                    log.info(
+                            "Environment-WorkerGroup relation create complete, environmentName:{}, workerGroup:{}.",
+                            env.getName(), relation.getWorkerGroup());


+                    relationMapper.insert(relation);
+                    log.info(
+                            "Environment-WorkerGroup relation create complete, environmentName:{}, workerGroup:{}.",
+                            env.getName(), relation.getWorkerGroup());


SbloodyS

** Preserve support for configuration-defined worker groups**

EnvironmentServiceImpl.java:446–453 treats any worker group absent from t_ds_worker_group as nonexistent. However, WorkerGroupServiceImpl.getConfigWorkerGroupPageDetail() discovers configuration-defined groups from worker heartbeats, and queryAllGroup() exposes them to the environment editor without requiring database rows.

Creating an environment with one of these valid groups now fails with WORKER_GROUP_NOT_EXIST. Updating an existing environment also fails even when only its configuration or description changes.

Please support configuration-defined groups when resolving relation identities, rather than requiring every selectable group to have a database row.
** Handle unmatched legacy relations before enforcing NOT NULL**

The PostgreSQL upgrade script at 3.5.0_schema/postgresql/dolphinscheduler_dml.sql:18–25 backfills IDs only when the stored worker group name matches t_ds_worker_group, then unconditionally applies SET NOT NULL. The MySQL migration applies the same constraint.

Existing relations to configuration-defined groups have no matching database row. Relations whose group was renamed can also remain unmatched. Their IDs stay NULL, so the PostgreSQL upgrade fails at the constraint change.

Please provide a migration strategy for these existing relations before enforcing NOT NULL, and keep the fresh-install schema consistent with how configuration-defined groups are represented.

[Improvement-17928][API] Add worker group id to environment relation

fe1f40c

qiuyanjun888 requested review from SbloodyS, caishunfeng and ruanwenjun as code owners June 14, 2026 13:05

github-actions Bot added backend test labels Jun 14, 2026

github-actions Bot assigned qiuyanjun888 Jun 14, 2026

ruanwenjun requested changes Jun 14, 2026

View reviewed changes

[Improvement-17928][API] Move worker group id backfill to DML

e7882b4

qiuyanjun888 requested a review from ruanwenjun June 15, 2026 02:23

Merge branch 'dev' into Improvement-17928

357458c

SbloodyS requested changes Jun 15, 2026

View reviewed changes

ruanwenjun reviewed Jun 15, 2026

View reviewed changes

qiuyanjun888 added 2 commits June 16, 2026 13:00

[Improvement-17928][API] Address environment worker group review

8268646

[Improvement-17928][API] Remove unrelated upgrade ddl newline change

05ef512

qiuyanjun888 requested review from SbloodyS and ruanwenjun June 16, 2026 05:46

SbloodyS requested changes Jun 16, 2026

View reviewed changes

[Improvement-17928][API] Address worker group mapper review

f592c03

qiuyanjun888 requested a review from SbloodyS June 16, 2026 08:15

qiuyanjun888 and others added 2 commits June 18, 2026 12:07

[Improvement-17928][API] Address worker group review feedback

31159fb

Merge branch 'dev' into Improvement-17928

be2c204

github-advanced-security Bot found potential problems Jul 1, 2026

View reviewed changes

SbloodyS reviewed Sep 7, 2026

View reviewed changes

		ALTER TABLE `t_ds_environment_worker_group_relation`
		ADD COLUMN `worker_group_id` int(11) DEFAULT NULL COMMENT 'worker group id'; No newline at end of file

		`worker_group_id` int(11) DEFAULT NULL COMMENT 'worker group id',
		`worker_group` varchar(255) NOT NULL COMMENT 'worker group name',

Uh oh!

Conversation

qiuyanjun888 commented Jun 14, 2026

Was this PR generated or assisted by AI?

Purpose of the pull request

Brief change log

Verify this pull request

Pull Request Notice

Uh oh!

ruanwenjun left a comment

Choose a reason for hiding this comment

Uh oh!

qiuyanjun888 commented Jun 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SbloodyS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants