[Feature-6471]Cache Process definition in master by lenboo · Pull Request #6485 · apache/dolphinscheduler

lenboo · 2021-10-10T14:31:10Z

close #6471 cache process definition in master

zhongjiajie

Add some of minor suggestion

sql/dolphinscheduler_mysql.sql

zhongjiajie

Seem not CI failed, but we have typo in our database filed

sql/dolphinscheduler_h2.sql

Co-authored-by: Jiajie Zhong <zhongjiajie955@hotmail.com>

…to dev-6471

codecov-commenter · 2021-10-11T08:13:22Z

Codecov Report

Merging #6485 (f8c765a) into dev (e091801) will increase coverage by 0.02%.
The diff coverage is 55.22%.

@@             Coverage Diff              @@
##                dev    #6485      +/-   ##
============================================
+ Coverage     38.62%   38.64%   +0.02%     
- Complexity     3206     3211       +5     
============================================
  Files           645      645              
  Lines         25735    25757      +22     
  Branches       2788     2792       +4     
============================================
+ Hits           9940     9955      +15     
- Misses        14881    14890       +9     
+ Partials        914      912       -2

Impacted Files	Coverage Δ
...inscheduler/server/master/config/MasterConfig.java	`15.38% <0.00%> (-1.29%)`	⬇️
...r/server/master/runner/MasterSchedulerService.java	`0.00% <0.00%> (ø)`
...inscheduler/service/quartz/ProcessScheduleJob.java	`0.00% <0.00%> (ø)`
...rg/apache/dolphinscheduler/dao/entity/Command.java	`43.79% <40.00%> (-0.11%)`	⬇️
...cheduler/api/service/impl/ExecutorServiceImpl.java	`48.72% <55.55%> (+0.46%)`	⬆️
...lphinscheduler/service/process/ProcessService.java	`37.22% <74.28%> (-1.02%)`	⬇️
...rver/master/processor/queue/TaskResponseEvent.java	`100.00% <0.00%> (+6.45%)`	⬆️
...er/master/processor/queue/TaskResponseService.java	`55.29% <0.00%> (+15.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e091801...f8c765a. Read the comment docs.

zhongjiajie · 2021-10-11T10:34:44Z

dolphinscheduler-server/src/main/resources/master.properties

 #master.task.commit.interval=1000

+# master cache process definition
+#master.cache.process.definition=true


Should we turn on cache by default

Suggested change

#master.cache.process.definition=true

master.cache.process.definition=true

we have already set the default value in the class MasterConfig.

For this situation, I think we better add comment in L42 to desc # master cache process definition, default true make it more clearer

you are right.
thank you for your suggestion.

caishunfeng · 2021-10-11T10:43:56Z

...r/src/main/java/org/apache/dolphinscheduler/server/master/runner/MasterSchedulerService.java

-                        this.masterConfig.getMasterExecThreads() - activeCount, command);
+                        command,
+                        processDefinitionCacheMaps);
+                if (!masterConfig.getMasterCacheProcessDefinition()


Is it better to manage processDefinition cache by a unified manager?

caishunfeng · 2021-10-11T10:47:10Z

...eduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessService.java

-        Map<String, String> cmdParam = JSONUtils.toMap(command.getCommandParam());
-
-        ProcessDefinition processDefinition = getProcessDefinitionByCommand(command.getProcessDefinitionCode(), cmdParam);
+        String key = String.format("%d-%d", command.getProcessDefinitionCode(), command.getProcessDefinitionVersion());


Any other else? Or it just use for constructProcessInstance?

Is it better to manage processDefinition cache by a unified manager?
yes, you are right, we can use a unified cache manager. we need to optimize this caching method next.

Any other else? Or it just use for constructProcessInstance?

maybe the task definitions can also be cached
and a clean-up mechanism can also be added to the cache queue, eg: clean cache queue once a day?

sonarqubecloud · 2021-10-11T11:16:47Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

48.5% Coverage
0.0% Duplication

zhongjiajie

LGTM now

* feature-6471 Cache Process definition in master

lenboo added 5 commits October 10, 2021 22:23

feature-6471 Cache Process definition in master

6aea382

update ut

a2a21d6

update ut

e407d4e

update ut

7c2bc45

add config for cache process definition

bd98389

zhongjiajie reviewed Oct 11, 2021

View reviewed changes

sql/dolphinscheduler_mysql.sql Show resolved Hide resolved

zhongjiajie reviewed Oct 11, 2021

View reviewed changes

sql/dolphinscheduler_h2.sql Outdated Show resolved Hide resolved

zhongjiajie mentioned this pull request Oct 11, 2021

[Feature][CI] Add sql file code style auto-formatter #6489

Closed

3 tasks

lenboo and others added 8 commits October 11, 2021 11:27

Update sql/dolphinscheduler_h2.sql

9af3456

Co-authored-by: Jiajie Zhong <zhongjiajie955@hotmail.com>

Merge branch 'dev-6471' of https://github.com/lenboo/EasyScheduler in…

66a8487

…to dev-6471

6417 add process-instance-id and process-definition-version

7de4ba0

unused import

5bf9c27

update ut

5d7eee6

update ut

a4466cf

update ut

4ad60fc

update ut

4980dc0

lenboo added 2 commits October 11, 2021 16:42

Merge remote-tracking branch 'upstream/dev' into dev-6471

d0e7311

remove transactional

e564a2b

zhongjiajie reviewed Oct 11, 2021

View reviewed changes

caishunfeng reviewed Oct 11, 2021

View reviewed changes

remove transactional

f8c765a

CalvinKirs approved these changes Oct 11, 2021

View reviewed changes

zhongjiajie approved these changes Oct 11, 2021

View reviewed changes

zhuangchong approved these changes Oct 11, 2021

View reviewed changes

zhuangchong merged commit db04a5b into apache:dev Oct 11, 2021

lenboo added a commit to lenboo/dolphinscheduler that referenced this pull request Oct 12, 2021

[Feature-6471]Cache Process definition in master (apache#6485)

d18105e

* feature-6471 Cache Process definition in master

lenboo mentioned this pull request Oct 12, 2021

[cherry-pick][6471-6502]cherry-pick 6471 && 6502 to 2.0-prepare #6508

Merged

	#master.cache.process.definition=true
	master.cache.process.definition=true

Conversation

lenboo commented Oct 10, 2021

Uh oh!

zhongjiajie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhongjiajie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

sonarqubecloud bot commented Oct 11, 2021

Uh oh!

zhongjiajie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Oct 11, 2021 •

edited

Loading