You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@dolphinscheduler.apache.org by GitBox <gi...@apache.org> on 2021/06/01 14:24:16 UTC
[GitHub] [dolphinscheduler] blackberrier opened a new pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
blackberrier opened a new pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572
<!--Thanks very much for contributing to Apache DolphinScheduler. Please review https://dolphinscheduler.apache.org/en-us/community/development/pull-request.html before opening a pull request.-->
## Purpose of the pull request
Check status of taskInstance from cache, and the cache refresh task state from database periodically.
## Brief change log
Master get task status from taskInstanceCacheManager instead of accessing database.
In taskInstanceCacheManager, I added a thread which query NEED_FAULT_TOLERANCE task from database and update the cache. To accelerate database access, I use indexed id from cache and check if its status equals NEED_FAULT_TOLERANCE.
## Verify this pull request
This pull request is code cleanup without any test coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644648690
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I think you can change the entire method to :),
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k ->processService.findTaskInstanceById(k));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter edited a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852697141
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5572](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2cae31a) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/cc9e5d5d34fcf2279b267cca7df37a9e80eeba07?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc9e5d5) will **decrease** coverage by `0.04%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## dev #5572 +/- ##
============================================
- Coverage 46.87% 46.82% -0.05%
+ Complexity 3792 3789 -3
============================================
Files 605 605
Lines 24800 24814 +14
Branches 2811 2813 +2
============================================
- Hits 11624 11620 -4
- Misses 12069 12086 +17
- Partials 1107 1108 +1
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/dolphinscheduler/common/Constants.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvY29tbW9uL0NvbnN0YW50cy5qYXZh) | `80.00% <ø> (ø)` | |
| [...aster/cache/impl/TaskInstanceCacheManagerImpl.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9jYWNoZS9pbXBsL1Rhc2tJbnN0YW5jZUNhY2hlTWFuYWdlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ler/server/master/runner/MasterTaskExecThread.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9ydW5uZXIvTWFzdGVyVGFza0V4ZWNUaHJlYWQuamF2YQ==) | `22.50% <0.00%> (ø)` | |
| [...er/master/dispatch/host/assign/RandomSelector.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9kaXNwYXRjaC9ob3N0L2Fzc2lnbi9SYW5kb21TZWxlY3Rvci5qYXZh) | `77.77% <0.00%> (-5.56%)` | :arrow_down: |
| [...er/master/processor/queue/TaskResponseService.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9wcm9jZXNzb3IvcXVldWUvVGFza1Jlc3BvbnNlU2VydmljZS5qYXZh) | `67.12% <0.00%> (-5.48%)` | :arrow_down: |
| [...uler/server/master/registry/ServerNodeManager.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9yZWdpc3RyeS9TZXJ2ZXJOb2RlTWFuYWdlci5qYXZh) | `79.13% <0.00%> (-0.87%)` | :arrow_down: |
| [...duler/server/registry/ZookeeperRegistryCenter.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL3JlZ2lzdHJ5L1pvb2tlZXBlclJlZ2lzdHJ5Q2VudGVyLmphdmE=) | `60.00% <0.00%> (ø)` | |
| [...inscheduler/service/zk/CuratorZookeeperClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kb2xwaGluc2NoZWR1bGVyL3NlcnZpY2UvemsvQ3VyYXRvclpvb2tlZXBlckNsaWVudC5qYXZh) | `75.60% <0.00%> (+4.87%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cc9e5d5...2cae31a](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] removed a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-854411675
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r643788658
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
Review comment:
Hi, @ruanwenjun I don't quite get the situation.
Assume that in one loop, we are checking the task instance, and at this time, `MasterTaskExecThread` remove it. At this point, 'remove' may mean two level, first is database remove, and we will get a null taskInstance. Second is cache remove, which means taskInstance is finished state. In both level, the 'if' condition is not satisfied.
Or another situation, one task is removed outside the entryset loop, then the task will not enter loop.
I wrote a demo for testing , listed below. In timer thread, the sleep simulate a time wasting opration, and another thread remove the entry while timer thread is doing the opration.
```
import java.util.Map;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ConcurrentHashMap;
public class ConcurrentHashMapRemoveIteratorTest {
private static ConcurrentHashMap<String, String> map = new ConcurrentHashMap<>();
public static void main(String[] args) {
/*for (int i = 0; i < 10; i++) {
map.put(i + ",", i + "");
}*/
map.put(10 + ",", 10 + "");
new Timer().scheduleAtFixedRate(
new TimerTask() {
@Override
public void run() {
System.out.println("new loop");
for (Map.Entry<String, String> entry : map.entrySet()) {
System.out.println("begin" + entry.getKey() + entry.getValue());
try {
System.out.println("sleeping");
Thread.sleep(5000);
} catch (InterruptedException e) {
e.printStackTrace();
}
System.out.println("end" + entry.getKey() + entry.getValue());
}
System.out.println("new loop end");
System.out.println();
}
}, 1000, 10000
);
new Thread(() -> {
try {
Thread.sleep(12000);
} catch (InterruptedException e) {
e.printStackTrace();
}
map.remove(10 + ",");
//map.put(10 + ",","?");
System.out.println("removed!");
}).start();
}
}
```
The output is
```
new loop
begin10,10
sleeping
end10,10
new loop end
new loop
begin10,10
sleeping
removed!
end10,10
new loop end
new loop
new loop end
```
Maybe there are something wrong in my situation. Can you point out? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644777130
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I know your meaning. I have checked the comment of the `computeIfAbsent`, it says as follows. So I wonder if the database access operation should take out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644703839
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I mean you can change this method to
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
}
```
This is an atomic operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644651163
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier You can change the entire method to
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
And you can add some ut.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-854857310
LGTM, can you write some unit test, the coverage is 0 now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r643929560
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
Review comment:
@ruanwenjun
I know what's wrong. I though NEED_FAULT_TOLERANCE tasks will not be remove in MasterExecThread. After I check, taskInstance.getState().typeIsFinished() include FAILURE NEED_FAULT_TOLERANCE. So you are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-857808932
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='77.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [77.3% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852697141
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5572](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1d7fe09) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/f8ecb536b71d6f33b71c73930832b62890b84ea1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f8ecb53) will **decrease** coverage by `0.03%`.
> The diff coverage is `5.26%`.
[](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## dev #5572 +/- ##
============================================
- Coverage 46.87% 46.84% -0.04%
Complexity 3792 3792
============================================
Files 605 605
Lines 24800 24816 +16
Branches 2811 2813 +2
============================================
- Hits 11626 11624 -2
- Misses 12068 12084 +16
- Partials 1106 1108 +2
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/dolphinscheduler/common/Constants.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvY29tbW9uL0NvbnN0YW50cy5qYXZh) | `80.00% <ø> (ø)` | |
| [...ler/server/master/runner/MasterTaskExecThread.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9ydW5uZXIvTWFzdGVyVGFza0V4ZWNUaHJlYWQuamF2YQ==) | `22.50% <0.00%> (ø)` | |
| [...aster/cache/impl/TaskInstanceCacheManagerImpl.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9jYWNoZS9pbXBsL1Rhc2tJbnN0YW5jZUNhY2hlTWFuYWdlckltcGwuamF2YQ==) | `2.22% <5.55%> (+2.22%)` | :arrow_up: |
| [...er/master/dispatch/host/assign/RandomSelector.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9kaXNwYXRjaC9ob3N0L2Fzc2lnbi9SYW5kb21TZWxlY3Rvci5qYXZh) | `77.77% <0.00%> (-5.56%)` | :arrow_down: |
| [...r/server/worker/processor/TaskCallbackService.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL3dvcmtlci9wcm9jZXNzb3IvVGFza0NhbGxiYWNrU2VydmljZS5qYXZh) | `38.09% <0.00%> (-3.18%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f8ecb53...1d7fe09](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644694052
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I think the database may waste time or something, and thus block other thread. So I take the `processService.findTaskInstanceById(k)` out
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r643614027
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
Review comment:
You should better not implement like this, if you want to close, you can only change the Stop, but Stop is used in many other places.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
+ }
+ }
+ Thread.sleep(CACHE_REFRESH_TIME_MILLIS);
Review comment:
You can use Timer to implement, rather than while(true) sleep
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/runner/MasterTaskExecThread.java
##########
@@ -149,7 +143,10 @@ public Boolean waitTaskQuit() {
this.checkTimeoutFlag = !alertTimeout();
}
// updateProcessInstance task instance
- taskInstance = processService.findTaskInstanceById(taskInstance.getId());
+ //taskInstance = processService.findTaskInstanceById(taskInstance.getId());
Review comment:
Remove the unused code.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
Review comment:
You need to consider the concurrency modification. For example, if there is a thread in `MasterTaskExecThread` remove the instance at the same time, these may cause a memory leak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-853057809
@ruanwenjun
Hi, I have updated my code.
1. use timer and scheduleAtFixedRate instead of while(stopper) and thread.sleep. And add close method as well.
2. add synchronized code segment to handler concurrent modification
3. remove useless code
Can you see the latest code? Review it please, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-854411675
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r645247377
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I mean you can use below code
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
}
```
The cache is used to reduce database pressure, in your plan, if there are multiple threads query the same taskInstance at the same time, they will all query from database.
Your consideration is that when conflicts occur, multiple thread query will block. I think if conflicts occur, if we don't want to be blocked, we should find a better hash method to reduce conflicts.
In additional, we can use one line of code to achieve, why use more complex code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644611806
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-854411675
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] codecov-commenter edited a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852697141
# [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5572](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2cae31a) into [dev](https://codecov.io/gh/apache/dolphinscheduler/commit/cc9e5d5d34fcf2279b267cca7df37a9e80eeba07?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cc9e5d5) will **decrease** coverage by `0.04%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## dev #5572 +/- ##
============================================
- Coverage 46.87% 46.82% -0.05%
+ Complexity 3792 3789 -3
============================================
Files 605 605
Lines 24800 24814 +14
Branches 2811 2813 +2
============================================
- Hits 11624 11620 -4
- Misses 12069 12086 +17
- Partials 1107 1108 +1
```
| [Impacted Files](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/dolphinscheduler/common/Constants.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvY29tbW9uL0NvbnN0YW50cy5qYXZh) | `80.00% <ø> (ø)` | |
| [...aster/cache/impl/TaskInstanceCacheManagerImpl.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9jYWNoZS9pbXBsL1Rhc2tJbnN0YW5jZUNhY2hlTWFuYWdlckltcGwuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ler/server/master/runner/MasterTaskExecThread.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9ydW5uZXIvTWFzdGVyVGFza0V4ZWNUaHJlYWQuamF2YQ==) | `22.50% <0.00%> (ø)` | |
| [...er/master/dispatch/host/assign/RandomSelector.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9kaXNwYXRjaC9ob3N0L2Fzc2lnbi9SYW5kb21TZWxlY3Rvci5qYXZh) | `77.77% <0.00%> (-5.56%)` | :arrow_down: |
| [...er/master/processor/queue/TaskResponseService.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9wcm9jZXNzb3IvcXVldWUvVGFza1Jlc3BvbnNlU2VydmljZS5qYXZh) | `67.12% <0.00%> (-5.48%)` | :arrow_down: |
| [...uler/server/master/registry/ServerNodeManager.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL21hc3Rlci9yZWdpc3RyeS9TZXJ2ZXJOb2RlTWFuYWdlci5qYXZh) | `79.13% <0.00%> (-0.87%)` | :arrow_down: |
| [...duler/server/registry/ZookeeperRegistryCenter.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2RvbHBoaW5zY2hlZHVsZXIvc2VydmVyL3JlZ2lzdHJ5L1pvb2tlZXBlclJlZ2lzdHJ5Q2VudGVyLmphdmE=) | `60.00% <0.00%> (ø)` | |
| [...inscheduler/service/zk/CuratorZookeeperClient.java](https://codecov.io/gh/apache/dolphinscheduler/pull/5572/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZG9scGhpbnNjaGVkdWxlci1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kb2xwaGluc2NoZWR1bGVyL3NlcnZpY2UvemsvQ3VyYXRvclpvb2tlZXBlckNsaWVudC5qYXZh) | `75.60% <0.00%> (+4.87%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [cc9e5d5...2cae31a](https://codecov.io/gh/apache/dolphinscheduler/pull/5572?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644825479
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I think the block may happen on two thread on two key, and the two key share the same hashcode, which means they are on the same Linkedlist or Red-black tree, and in `computeIfAbsent` synchronized lock on the head node.
If we write like this,
```
taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
and if the first key access database and wasting time , then the second key have to wait.
If we take database access out, like
```
taskInstance = processService.findTaskInstanceById(taskInstanceId);
TaskInstance finalTaskInstance = taskInstance;
taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
```
the second key need not wait while the first key is accessing database.
I wonder if my thought is reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644846544
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier
> the block may happen on two thread on two key, and the two key share the same hashcode
Yes, but we can avoid the 1 step.
```
1. taskInstance = processService.findTaskInstanceById(taskInstanceId);
2. TaskInstance finalTaskInstance = taskInstance;
3. taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852699106
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='3.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [3.8% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-857338643
@blackberrier Great, I see your job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] removed a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852699106
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='3.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [3.8% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] CalvinKirs commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852275921
@ruanwenjun PTAL.Thx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r647941173
##########
File path: dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImplTest.java
##########
@@ -0,0 +1,160 @@
+package org.apache.dolphinscheduler.server.master.cache.impl;
Review comment:
ok, I've added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r647935369
##########
File path: dolphinscheduler-server/src/test/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImplTest.java
##########
@@ -0,0 +1,160 @@
+package org.apache.dolphinscheduler.server.master.cache.impl;
Review comment:
You need to add Apache license header here, you can find the license header in other file.
And when you add an ut file, you need to add the test class in root pom.xml like below:
```java
<include>**/api/service/MonitorServiceTest.java</include>
<include>**/api/service/ProcessDefinitionServiceTest.java</include>
<include>**/api/service/ProcessTaskRelationServiceImplTest.java</include>
<include>**/api/service/TaskDefinitionServiceImplTest.java</include>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644648690
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I think you can change the entire method to :),
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k ->processService.findTaskInstanceById(k));
```
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier You can change the entire method to
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier You can change the entire method to
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
And you can add some ut.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I mean you can change this method to
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
}
```
This is an atomic operation.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier
If there are two threads execute `getByTaskInstanceId` at the same time, we just want to query from database once, right? The block will only happen on the two thread update on the same key, and the key is not exist, this is expected.
And you should realize that the below way is not an atomic operation, in some situation, it might cause problem.
```java
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
if (taskInstance == null){
taskInstance = processService.findTaskInstanceById(taskInstanceId);
taskInstanceCache.put(taskInstanceId,taskInstance);
}
return taskInstance;
```
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier
> the block may happen on two thread on two key, and the two key share the same hashcode
Yes, but we can avoid the 1 step.
```
1. taskInstance = processService.findTaskInstanceById(taskInstanceId);
2. TaskInstance finalTaskInstance = taskInstance;
3. taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
```
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier I mean you can use below code
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
}
```
The cache is used to reduce database pressure, in your plan, if there are multiple threads query the same taskInstance at the same time, they will all query from database.
Your consideration is that when conflicts occur, multiple thread query will block. I think if conflicts occur, if we don't want to be blocked, we should find a better hash method to reduce conflicts.
In additional, we can use one line of code to achieve, why use more complex code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] CalvinKirs merged pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
CalvinKirs merged pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-857336870
> LGTM, can you write some unit tests, the coverage is 0 now.
@ruanwenjun
Hi, I've added some unit tests for `TaskInstanceCacheManagerImpl`. Please review it for me. Thx.
By the way, while I am working on this, I have found a bug in `cacheTaskInstance(TaskExecuteResponseCommand taskExecuteResponseCommand)` method , and I have fixed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] CalvinKirs commented on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
CalvinKirs commented on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-857805003
Deeply thanks for your contribution, considering that it's your first contribution, I think we can get deep communication, you can contact me through mail or add WeChat(Kris_Evil), when mail or added, please tell me who you are, I think I can help to familiar with the DolphinScheduler if you meet with problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r643939324
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
Review comment:
@blackberrier The real process should be that the task might be removed from cache first, then add to cache again, and then it will always exist in cache, this cause memory leak 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644651163
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier You can change the entire method to
```
return taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun edited a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun edited a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-854857310
LGTM, can you write some unit tests, the coverage is 0 now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644608903
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -114,6 +143,23 @@ public void cacheTaskInstance(TaskExecuteResponseCommand taskExecuteResponseComm
*/
@Override
public void removeByTaskInstanceId(Integer taskInstanceId) {
- taskInstanceCache.remove(taskInstanceId);
+ synchronized (lock) {
+ taskInstanceCache.remove(taskInstanceId);
+ }
+ }
+
+ class RefreshTaskInstanceTimerTask extends TimerTask {
+ @Override
+ public void run() {
+ synchronized (lock) {
Review comment:
cool
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r645698526
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
ok, I have commit latest code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r643830987
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -47,6 +58,33 @@
@Autowired
private ProcessService processService;
+ @PostConstruct
+ public void init() {
+ super.setName("TaskInstanceCacheRefreshThread");
+ super.start();
+ }
+
+ /**
+ * issue#5539 add thread to fetch task state from database in a fixed rate
+ */
+ @Override
+ public void run() {
+ while (Stopper.isRunning()) {
+ try {
+ for (Entry<Integer, TaskInstance> taskInstanceEntry : taskInstanceCache.entrySet()) {
+ TaskInstance taskInstance = processService.findTaskInstanceById(taskInstanceEntry.getKey());
+ if (null != taskInstance && taskInstance.getState() == ExecutionStatus.NEED_FAULT_TOLERANCE) {
+ logger.debug("task {} need fault tolerance, update instance cache", taskInstance.getId());
+ taskInstanceCache.put(taskInstanceEntry.getKey(), taskInstance);
Review comment:
@blackberrier
This is not an atomic operation.
When you remove the task in MasterTaskExecThread, but then at the same time you query from database and add the task to cache.
Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] sonarcloud[bot] removed a comment on pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#issuecomment-852699106
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/C.png' alt='C' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache-dolphinscheduler&pullRequest=5572&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='3.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list) [3.8% Coverage](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache-dolphinscheduler&pullRequest=5572&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644608903
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -114,6 +143,23 @@ public void cacheTaskInstance(TaskExecuteResponseCommand taskExecuteResponseComm
*/
@Override
public void removeByTaskInstanceId(Integer taskInstanceId) {
- taskInstanceCache.remove(taskInstanceId);
+ synchronized (lock) {
+ taskInstanceCache.remove(taskInstanceId);
+ }
+ }
+
+ class RefreshTaskInstanceTimerTask extends TimerTask {
+ @Override
+ public void run() {
+ synchronized (lock) {
Review comment:
cool
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I think the database may waste time or something, and thus block other thread. So I take the `processService.findTaskInstanceById(k)` out
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I know your meaning. I have checked the comment of the `computeIfAbsent`, it says as follows. So I wonder if the database access operation should take out.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I think the block may happen on two thread on two key, and the two key share the same hashcode, which means they are on the same Linkedlist or Red-black tree, and in `computeIfAbsent` synchronized lock on the head node.
If we write like this,
```
taskInstanceCache.computeIfAbsent(taskInstanceId, k -> processService.findTaskInstanceById(k));
```
and if the first key access database and wasting time , then the second key have to wait.
If we take database access out, like
```
taskInstance = processService.findTaskInstanceById(taskInstanceId);
TaskInstance finalTaskInstance = taskInstance;
taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
```
the second key need not wait while the first key is accessing database.
I wonder if my thought is reasonable.
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I can't quite understand.
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
if (taskInstance == null) {
1. taskInstance = processService.findTaskInstanceById(taskInstanceId);
2. TaskInstance finalTaskInstance = taskInstance;
3. taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
}
return taskInstance;
}
```
If we remove the 1 step, then where can we get the taskInstance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644075530
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -114,6 +143,23 @@ public void cacheTaskInstance(TaskExecuteResponseCommand taskExecuteResponseComm
*/
@Override
public void removeByTaskInstanceId(Integer taskInstanceId) {
- taskInstanceCache.remove(taskInstanceId);
+ synchronized (lock) {
+ taskInstanceCache.remove(taskInstanceId);
+ }
+ }
+
+ class RefreshTaskInstanceTimerTask extends TimerTask {
+ @Override
+ public void run() {
+ synchronized (lock) {
Review comment:
I think you can remove lock, use `computeIfPresent` if present to update the status
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
It is better to use `computeIfAbsent` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644611806
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
I put database access out of `computeIfAbsent`'s `Function` class `apply` method, is this ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] ruanwenjun commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644788688
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@blackberrier
If there are two threads execute `getByTaskInstanceId` at the same time, we just want to query from database once, right? The block will only happen on the two thread update on the same key, and the key is not exist, this is expected.
And you should realize that the below way is not an atomic operation, in some situation, it might cause problem.
```java
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
if (taskInstance == null){
taskInstance = processService.findTaskInstanceById(taskInstanceId);
taskInstanceCache.put(taskInstanceId,taskInstance);
}
return taskInstance;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [dolphinscheduler] blackberrier commented on a change in pull request #5572: [Improvement-5539][Master] Check status of taskInstance from cache
Posted by GitBox <gi...@apache.org>.
blackberrier commented on a change in pull request #5572:
URL: https://github.com/apache/dolphinscheduler/pull/5572#discussion_r644870000
##########
File path: dolphinscheduler-server/src/main/java/org/apache/dolphinscheduler/server/master/cache/impl/TaskInstanceCacheManagerImpl.java
##########
@@ -57,7 +86,7 @@
@Override
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
Review comment:
@ruanwenjun
I can't quite understand.
```
public TaskInstance getByTaskInstanceId(Integer taskInstanceId) {
TaskInstance taskInstance = taskInstanceCache.get(taskInstanceId);
if (taskInstance == null) {
1. taskInstance = processService.findTaskInstanceById(taskInstanceId);
2. TaskInstance finalTaskInstance = taskInstance;
3. taskInstanceCache.computeIfAbsent(taskInstanceId, k -> finalTaskInstance);
}
return taskInstance;
}
```
If we remove the 1 step, then where can we get the taskInstance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org