Skip to content

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

Closed
sdhzwc wants to merge 28 commits intoapache:devfrom
sdhzwc:Improvement-16443
Closed

[Improvement-16443][Workflow Timing] Improvement add operator#16450
sdhzwc wants to merge 28 commits intoapache:devfrom
sdhzwc:Improvement-16443

Conversation

@sdhzwc
Copy link
Contributor

@sdhzwc sdhzwc commented Aug 13, 2024 •
edited
Loading

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

@github-actions github-actions bot added UI ui and front end related backend labels Aug 13, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Aug 14, 2024
@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Aug 14, 2024
@sdhzwc sdhzwc requested a review from SbloodyS August 14, 2024 05:49
SbloodyS
SbloodyS previously approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ruanwenjun
ruanwenjun reviewed Aug 19, 2024
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sdhzwc
Copy link
Contributor Author

sdhzwc commented Aug 19, 2024 •
edited
Loading

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

What needs to be adjusted? Could you specify?

@ruanwenjun
Copy link
Member

ruanwenjun commented Aug 19, 2024

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
Copy link
Member

ruanwenjun commented Aug 19, 2024 •
edited
Loading

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.

@sdhzwc
Copy link
Contributor Author

sdhzwc commented Aug 20, 2024

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.

@ruanwenjun
Copy link
Member

ruanwenjun commented Aug 20, 2024

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.

@sdhzwc
Copy link
Contributor Author

sdhzwc commented Aug 20, 2024

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 sdhzwc requested a review from ruanwenjun August 20, 2024 03:25
topsli and others added 6 commits August 21, 2024 10:15
@topsli
[Fix-16475][UI] Can't SET Timing successful in Workflow Definition gr…
5ffa452 
…id (#16476)
@saowu @SbloodyS
[Fix-16492][alert] FeiShuSender Exception.getMessage is null (#16498)
219a232 
* Update FeiShuSender.java

The Exception.getMessage present here is null, and the problem has occurred

* Update dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-feishu/src/main/java/org/apache/dolphinscheduler/plugin/alert/feishu/FeiShuSender.java

Co-authored-by: xiangzihao <zihaoxiang@apache.org>

---------

Co-authored-by: xiangzihao <zihaoxiang@apache.org>
@BruceWong96
[Doc-16490] Changed some notes about python gateway service (#16493)
d7f950c
@shouwangyw
[Improvement-16487][Master] Improve the dependent result for workflow…
10295d3 
… dependency tasks (#16488)
@lyyprean
[Improvement-16501][dolphinscheduler-api]typo in Status.java（#16501） (#…
c99de20 
…16504)
@cncws
[Fix-15961] [UI] Rename pythonCommand to pythonLauncher in pytorch ta…
e8098ac 
…sk. (#16495)
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 25, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

ruanwenjun and others added 13 commits August 26, 2024 15:19
@ruanwenjun
[DSIP-61][Master] Refactor thread pool and state event orchestration …
9448806 
…in master (#16327)
@ruanwenjun
[DSIP-67] Use command to trigger workflow instance rather generate wo…
20a0e50 
…rkflow instance (#16523)
@SbloodyS
[Improvement-16516] Uniformly name process in backend code as workflow (
4c19506 
#16524)
@pegasas
[Improvement][API] Fix typo on api workflow backfill (#16538)
56af000
@wangxj3
[Doc-16535] remove doc of extra ssh task node type (#16536)
833fb5b
@SbloodyS
remove staleness code owner (#16547)
4fb0cb8
@ruanwenjun
[Improve] Move recover/repeat running to IWorkflowControlClient (#16545)
002e9ca
@SbloodyS
[DSIP-70] Deprecated upgrade code of 1.x and 2.x in 3.3.0-release (#1…
ceb711b 
…6551)
@ruanwenjun @SbloodyS
[Chore] Remove unsupported feature in README.md (#16549)
d64837d 
* [Chore] Remove unsupported feature in README.md

* Update README_zh_CN.md

Co-authored-by: xiangzihao <zihaoxiang@apache.org>

---------

Co-authored-by: xiangzihao <zihaoxiang@apache.org>
Co-authored-by: xiangzihao <460888207@qq.com>
@SbloodyS
[Chore] Improve release doc (#16553)
deddb3b 
* improve release doc
[Improvement-16443][Workflow Timing] Improvement add operator
0d49ae7
operator changed to modify_user_id
58dbf79
Merge remote-tracking branch 'origin/Improvement-16443' into Improvem…
2b4be27 
…ent-16443

# Conflicts:
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/SchedulerServiceImpl.java
#	dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/mysql/dolphinscheduler_ddl.sql
#	dolphinscheduler-dao/src/main/resources/sql/upgrade/3.3.0_schema/postgresql/dolphinscheduler_ddl.sql
@sdhzwc sdhzwc requested a review from Gallardot as a code owner August 31, 2024 06:41
@sdhzwc sdhzwc closed this Aug 31, 2024
@sdhzwc sdhzwc deleted the Improvement-16443 branch August 31, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SbloodyS SbloodyS SbloodyS left review comments

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

@songjianet songjianet Awaiting requested review from songjianet songjianet is a code owner

@Amy0104 Amy0104 Awaiting requested review from Amy0104

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@ruanwenjun ruanwenjun Awaiting requested review from ruanwenjun ruanwenjun is a code owner

@Gallardot Gallardot Awaiting requested review from Gallardot Gallardot is a code owner

Assignees

@sdhzwc sdhzwc

Labels

backend CI&CD document e2e e2e test improvement make more easy to user or prompt friendly test UI ui and front end related

Projects

None yet

Milestone

3.3.0-alpha

Development

Successfully merging this pull request may close these issues.

[Improvement][Workflow Timing] need for additional operators