Skip to content

[BUG#5984]fix multi task delete issue#5983

Closed
caoyj1991 wants to merge 21 commits intoapache:devfrom
caoyj1991:multi-instance-delete
Closed

[BUG#5984]fix multi task delete issue#5983
caoyj1991 wants to merge 21 commits intoapache:devfrom
caoyj1991:multi-instance-delete

Conversation

@caoyj1991
Copy link
Copy Markdown
Contributor

@caoyj1991 caoyj1991 commented Aug 12, 2021

Purpose of the pull request

Avoid a large number of nested iterative deletions, separate the log file deletion operation from the database cleanup operation, and ensure that the file deletions of the same host are grouped together. l Once host deletes all files on this host.

避免產生大量的嵌套迭代的刪除情況,將日志文件的刪除操作和數據庫清理操作分離,并且保證同一個host 的文件刪除聚合在一起。l連一次host 刪除這個host 上所有文件。

Verify this pull request

This pull request is code cleanup without any test coverage.

compile pass

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:

@caoyj1991
Copy link
Copy Markdown
Contributor Author

#5984

@CalvinKirs CalvinKirs changed the title fix multi task delete issue [BUG#5984]fix multi task delete issue Aug 12, 2021
@CalvinKirs CalvinKirs added bug Something isn't working first time contributor First-time contributor labels Aug 12, 2021
@caoyj1991 caoyj1991 closed this Aug 13, 2021
@caoyj1991 caoyj1991 reopened this Aug 13, 2021
Copy link
Copy Markdown
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.

In my perspective, the main change of this pr is adding batch remove logs operation.
This can reduce the rpc calls, it's ok.

But I am not sure using rm -rf paths is better than using jdk's file api directly.

@CalvinKirs
Copy link
Copy Markdown
Member

hi, thank you very much for your contribution. We only accept discussions in English. We suggest you make changes to the Chinese content.

@caoyj1991 caoyj1991 closed this Aug 17, 2021
@caoyj1991 caoyj1991 reopened this Aug 17, 2021
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 17, 2021

Codecov Report

❌ Patch coverage is 9.27835% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.20%. Comparing base (f7e0e9f) to head (1c45220).
⚠️ Report is 3822 commits behind head on dev.

Files with missing lines Patch % Lines
...lphinscheduler/service/process/ProcessService.java 0.00% 31 Missing ⚠️
...inscheduler/server/log/LoggerRequestProcessor.java 16.00% 21 Missing ⚠️
...phinscheduler/common/thread/AsyncStreamThread.java 0.00% 16 Missing ⚠️
...uler/api/controller/ProcessInstanceController.java 0.00% 9 Missing ⚠️
...r/api/service/impl/ProcessInstanceServiceImpl.java 16.66% 5 Missing ⚠️
...pache/dolphinscheduler/common/cmd/LinuxSystem.java 0.00% 3 Missing ⚠️
...emote/command/log/RemoveTaskLogRequestCommand.java 50.00% 2 Missing ⚠️
...dolphinscheduler/service/log/LogClientService.java 66.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #5983      +/-   ##
============================================
- Coverage     46.75%   46.20%   -0.56%     
+ Complexity     3743     3711      -32     
============================================
  Files           604      606       +2     
  Lines         24820    24888      +68     
  Branches       2832     2842      +10     
============================================
- Hits          11605    11499     -106     
- Misses        12074    12262     +188     
+ Partials       1141     1127      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

+1
good job, Look forward to making more contributions

@davidzollo davidzollo requested a review from ruanwenjun August 21, 2021 16:50
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

And there are a lot of code style changes that are not better than don't to change them

Comment on lines +113 to +115
for (String path : taskLogPaths) {
cmd += os.deleteCmd() + path + ";";
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I personally don't think manipulating the command is better than JDK APIs, and you should know manipulating the strings to a system command is dangerous and can be injected malicious codes as well

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

其他的部分我倒是能理解。毕竟是追求比较优质的代码。

第一,ds 的使用场景并不是对外的,而是对内的。
第二,如果存在安全漏洞,其实基本可以认为是不太可能的。因为删除的文件名是来自数据库内自动生成的文件,如果会有可注入的风险,就是因为数据的库的信息被串改了。完全属于没病呻吟的情况。
第三,作为DS 4个版本的使用者,我要说一句,现在批量删除的逻辑是有可能导致 这个系统异常的。日志本身在运维阶段属于可以被定期清理的部分。如果UI 操作删除批量日志能导致使用者直接面对系统宕机,系统本身的稳定性是需要紧急优化的。
第四,非得强调jdk 提供的API 才是最安全的这个情况。个人不是没有考虑过服用jdk 的API 。 但是在牺牲一次性删除的方案下,找个看着可能对的方法去做,并不是解决问题的方式。当然可以说开启异步线程去清理。。。但是有必要吗?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

请不要按照开发人员的方式去思考用户该怎么用,因为官方文档里也没有限制用户可嵌套的层级。

后期我个人带着团队用的时候都是直接删除数据库的。

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

第一,ds 的使用场景并不是对外的,而是对内的。

Company employees are totally possible to destroy their servers / databases.

第二,如果存在安全漏洞,其实基本可以认为是不太可能的。因为删除的文件名是来自数据库内自动生成的文件,如果会有可注入的风险,就是因为数据的库的信息被串改了。完全属于没病呻吟的情况。

Do you mean if the database is hacked, it's reasonable that your servers should be also hacked? You should limit the exploding radius instead of expanding it.

第三,作为DS 4个版本的使用者,我要说一句,现在批量删除的逻辑是有可能导致 这个系统异常的。日志本身在运维阶段属于可以被定期清理的部分。如果UI 操作删除批量日志能导致使用者直接面对系统宕机,系统本身的稳定性是需要紧急优化的。

You said the original logic is possible to cause exceptions, but didn't explain what exceptions might be caused and why your changes will resolve it.

第四,非得强调jdk 提供的API 才是最安全的这个情况。个人不是没有考虑过服用jdk 的API 。 但是在牺牲一次性删除的方案下,找个看着可能对的方法去做,并不是解决问题的方式。当然可以说开启异步线程去清理。。。但是有必要吗?
请不要按照开发人员的方式去思考用户该怎么用,因为官方文档里也没有限制用户可嵌套的层级。
后期我个人带着团队用的时候都是直接删除数据库的。

Please don't take it personally and don't be so aggressive, reviewers just give their opinions and raise their concerns, giving your reasons is just enough, that's how open source community works, again, don't take it personally, don't be aggressive.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm OK to just give a +1 if you don't like your codes reviewed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

原因就在最初的介绍里。
image

有效的建议个人认为是都是比较正向可以接受的。但是如果不清楚整体情况下反复的重复同样的问题,回复同样的解释。貌似并不是很有效的沟通,这个PR 上已经回复了第二次这个问题的起因和解决方法的设计。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

@sonarqubecloud
Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

8.5% 8.5% Coverage
0.0% 0.0% Duplication

@caoyj1991 caoyj1991 closed this Aug 22, 2021
@caoyj1991 caoyj1991 deleted the multi-instance-delete branch August 22, 2021 14:38
@CalvinKirs
Copy link
Copy Markdown
Member

CalvinKirs commented Aug 24, 2021

I'm sorry this PR was closed, if you have a suitable solution later please feel free to discuss and contribute, of course, it's up to you.
But I have two suggestions:

First of all:Please use English, as I mentioned above and as you have noticed, all reviewers use English,no matter where they come from, And I see that you have replied to them.It's important to follow the community rules.

Secondly:In open source community code contributions, anyone has the right to review, and the code submitted by anyone must be reviewed, whether it is chair or user. it's normal for people to argue, and I don't really want to discuss right or wrong here.but It's important to keep peaceful communication, and the Apache Way emphasizes "respectful, honest, technical-based interaction".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working first time contributor First-time contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants