dolphinscheduler icon indicating copy to clipboard operation
dolphinscheduler copied to clipboard

[Improvement-16443][Workflow Timing] Improvement add operator

Open sdhzwc opened this issue 1 year ago • 7 comments

close #16443 The effect is as follows: recording

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

sdhzwc avatar Aug 13 '24 10:08 sdhzwc

It's better to set modify_user as create_user when upgrade.

What needs to be adjusted? Could you specify?

sdhzwc avatar Aug 19 '24 03:08 sdhzwc

It's better to set modify_user as create_user when upgrade.

What needs to be adjusted? Could you specify?

It's not a good idea to make modify_user_id DEFAULT NULL, this will make confusion, why the create user is not null, but the modify_user_id can be null, no one can explain this. If the modify_user_id is null is this a bug?

To avoid this kind problem, you need to inject a value for the new column, obviously you should use create_user to inject.

ruanwenjun avatar Aug 19 '24 15:08 ruanwenjun

Additional, it's better to create a DSIP to unify the logic, we should add modify user to all table which have create user. Otherwise, this look like strange.

ruanwenjun avatar Aug 19 '24 15:08 ruanwenjun

It's better to set modify_user as create_user when upgrade.

What needs to be adjusted? Could you specify?

It's not a good idea to make modify_user_id DEFAULT NULL, this will make confusion, why the create user is not null, but the modify_user_id can be null, no one can explain this. If the modify_user_id is null is this a bug?

To avoid this kind problem, you need to inject a value for the new column, obviously you should use create_user to inject.

The reason is very simple. When the timer is created, it is definite. The creator is the logged-in person. At this time, there is no modification operation, so there is no need to specify the updater. I think it makes sense. If the updater is added, I think there will be ambiguity because there is no update operation yet.

sdhzwc avatar Aug 20 '24 01:08 sdhzwc

It's better to set modify_user as create_user when upgrade.

What needs to be adjusted? Could you specify?

It's not a good idea to make modify_user_id DEFAULT NULL, this will make confusion, why the create user is not null, but the modify_user_id can be null, no one can explain this. If the modify_user_id is null is this a bug? To avoid this kind problem, you need to inject a value for the new column, obviously you should use create_user to inject.

The reason is very simple. When the timer is created, it is definite. The creator is the logged-in person. At this time, there is no modification operation, so there is no need to specify the updater. I think it makes sense. If the updater is added, I think there will be ambiguity because there is no update operation yet.

This is not a common practice, please reference the last_modify_user/last_modify_time in aws/linux.

ruanwenjun avatar Aug 20 '24 02:08 ruanwenjun

It's better to set modify_user as create_user when upgrade.

What needs to be adjusted? Could you specify?

It's not a good idea to make modify_user_id DEFAULT NULL, this will make confusion, why the create user is not null, but the modify_user_id can be null, no one can explain this. If the modify_user_id is null is this a bug? To avoid this kind problem, you need to inject a value for the new column, obviously you should use create_user to inject.

The reason is very simple. When the timer is created, it is definite. The creator is the logged-in person. At this time, there is no modification operation, so there is no need to specify the updater. I think it makes sense. If the updater is added, I think there will be ambiguity because there is no update operation yet.

This is not a common practice, please reference the last_modify_user/last_modify_time in aws/linux.

It has been changed. The value has been added when inserting.

sdhzwc avatar Aug 20 '24 03:08 sdhzwc

Please retry analysis of this Pull-Request directly on SonarCloud

sonarqubecloud[bot] avatar Aug 25 '24 01:08 sonarqubecloud[bot]