Skip to content

[Bug][Master] Fix taskInstance's start time might be null when dispatch failed#15290

Closed
jackyyyyyssss wants to merge 2 commits intoapache:devfrom
jackyyyyyssss:fix_time
Closed

[Bug][Master] Fix taskInstance's start time might be null when dispatch failed#15290
jackyyyyyssss wants to merge 2 commits intoapache:devfrom
jackyyyyyssss:fix_time

Conversation

@jackyyyyyssss
Copy link
Contributor

Purpose of the pull request

If there is only one work and the available resources are very low, the heartbeat will result in no available work Dispatcher method reporting an error, and calling addDispatchFailedEvent will result in an empty starttime. In this case, the task is actually a failure. When querying failed tasks in the foreground, the count is incorrect because the starttime is empty
image
image

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

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

(or)

This change added tests and can be verified as follows:

(or)

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

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

@ruanwenjun ruanwenjun added the bug Something isn't working label Dec 7, 2023
@ruanwenjun ruanwenjun changed the title [Bug][Master] starttime is null [Bug][Master] Fix taskInstance's start time might be null when dispatch failed Dec 7, 2023
@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (0bb48f3) 38.19% compared to head (abbb342) 38.19%.

❗ Current head abbb342 differs from pull request most recent head 6ca5ab8. Consider uploading reports for the commit 6ca5ab8 to get more accurate results

Files Patch % Lines
...duler/server/master/runner/BaseTaskDispatcher.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##                dev   #15290   +/-   ##
=========================================
  Coverage     38.19%   38.19%           
  Complexity     4673     4673           
=========================================
  Files          1278     1278           
  Lines         44481    44481           
  Branches       4783     4783           
=========================================
  Hits          16988    16988           
  Misses        25631    25631           
  Partials       1862     1862           

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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2023

Please retry analysis of this Pull-Request directly on SonarCloud.

fuchanghai

This comment was marked as outdated.

Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

@jackyyyyyssss
Copy link
Contributor Author

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

Thank you for your feedback. It is incorrect to have a start and end time according to this method, but not setting a time will result in the loss of front-end statistical tasks

@fuchanghai
Copy link
Member

LGTM

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

Thank you for your feedback. It is incorrect to have a start and end time according to this method, but not setting a time will result in the loss of front-end statistical tasks

hi @jackyyyyyssss can we modify the statistical's interface to fix this bug?

@jackyyyyyssss
Copy link
Contributor Author

LGTM

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

Thank you for your feedback. It is incorrect to have a start and end time according to this method, but not setting a time will result in the loss of front-end statistical tasks

hi @jackyyyyyssss can we modify the statistical's interface to fix this bug?
Statistical task time is a display field. If it is empty, it may seem strange. We can take a look at the opinions of others

@fuchanghai
Copy link
Member

LGTM

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

Thank you for your feedback. It is incorrect to have a start and end time according to this method, but not setting a time will result in the loss of front-end statistical tasks

hi @jackyyyyyssss can we modify the statistical's interface to fix this bug?
Statistical task time is a display field. If it is empty, it may seem strange. We can take a look at the opinions of others

Can you tell me which statistics interface it is? I'd be happy to take a look at it when I have free time.

@jackyyyyyssss
Copy link
Contributor Author

LGTM

hi @jackyyyyyssss I refute the point I agreed with before, because according to the logic of the previous version, startTime is the time when the task is in the running state, and startTime in the dispatch state should be null because the task has not really started. We should solve it in another way. this problem , WDYT? cc @ruanwenjun

Thank you for your feedback. It is incorrect to have a start and end time according to this method, but not setting a time will result in the loss of front-end statistical tasks

hi @jackyyyyyssss can we modify the statistical's interface to fix this bug?
Statistical task time is a display field. If it is empty, it may seem strange. We can take a look at the opinions of others

Can you tell me which statistics interface it is? I'd be happy to take a look at it when I have free time.
Display on the homepage

image

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

Labels

backend bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants