Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix-#13956] [Master]taskId is null cause NPE #13980

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

fuchanghai
Copy link
Member

@fuchanghai fuchanghai commented Apr 21, 2023

Purpose of the pull request

this close

Brief change log

This judgment seems to be on the surface if taskInstacne is null, only to enter,
after task insert failed , id is null
image
I remember that id used to be of type int, but it was later changed to type Integer. My personal opinion is that the above reason caused the exception.

Verify this pull request

@fuchanghai fuchanghai changed the title [improve-#13956] [Master]taskId is null cause NPE [fix-#13956] [Master]taskId is null cause NPE Apr 21, 2023
@fuchanghai
Copy link
Member Author

@caishunfeng @SbloodyS @ruanwenjun PTAL

Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM

@caishunfeng caishunfeng added the bug Something isn't working label Apr 21, 2023
@caishunfeng caishunfeng added 3.1.x for 3.1.x version 3.2.0 for 3.2.0 version labels Apr 21, 2023
@SbloodyS SbloodyS added this to the 3.1.6 milestone Apr 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

Merging #13980 (e87d7e4) into dev (26c5f3c) will increase coverage by 0.01%.
The diff coverage is 0.00%.

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

@@             Coverage Diff              @@
##                dev   #13980      +/-   ##
============================================
+ Coverage     38.91%   38.93%   +0.01%     
- Complexity     4469     4472       +3     
============================================
  Files          1164     1164              
  Lines         42444    42445       +1     
  Branches       4761     4761              
============================================
+ Hits          16518    16526       +8     
+ Misses        24106    24096      -10     
- Partials       1820     1823       +3     
Impacted Files Coverage Δ
.../server/master/runner/WorkflowExecuteRunnable.java 9.76% <0.00%> (-0.01%) ⬇️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarcloud
Copy link

sonarcloud bot commented Apr 21, 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

@SbloodyS SbloodyS merged commit d617620 into apache:dev Apr 21, 2023
@fuchanghai fuchanghai deleted the fix-#13956 branch April 26, 2023 07:43
@zhuangchong
Copy link
Contributor

The taskId cannot be set to 0 here, because the task instance will be submitted and written to the database before this, thereby generating the id.
https://github.com/fuchanghai/dolphinscheduler/blob/ef81911fe55f19faed00a4e908c0ce1c3d7e39ef/dolphinscheduler-service/src/main/java/org/apache/dolphinscheduler/service/process/ProcessServiceImpl.java#L1159-L1165

@zuzuviewer
Copy link

zuzuviewer commented May 4, 2023

I think bug is from here, created new task instance but not insert to db and task instance id is null
dsbug1
dsbug2
dsbug3

@zhuangchong
Copy link
Contributor

@zuzuviewer Yes, the specific reason is that the task instance creation failed because the obtained resource data is empty.

@zuzuviewer
Copy link

@zuzuviewer Yes, the specific reason is that the task instance creation failed because the obtained resource data is empty.

@zhuangchong what resource data

@zuzuviewer
Copy link

How about this fix
fixds

@HomminLee
Copy link
Contributor

HomminLee commented May 6, 2023

@fuchanghai
I reproduce this BUG, according to #12440 (comment). I'm sure this cause by resource id。

After debugger, I found the problem lies in three steps:

  1. The resource files referenced by offline workflow could be deleted in Resource center, but the id of deleted resource files still be used when the workflow is edited and saved.
    1683367133680

  2. Before submitTaskInstanceToDB, will update resource info in task instance with no check exists, so throw NPE.
    1683364568613

  3. submitTaskExec will catch the exception, judge this is due to submit failed. Code go on, but task instance has not save to BD, so there are loop NPE logs.
    1683364661997

@zhuangchong
Copy link
Contributor

Version 3.1.6 will fix this issue.
#13878

@HomminLee
Copy link
Contributor

Version 3.1.6 will fix this issue. #13878

Does the UI also need to be fixed?

@zhuangchong
Copy link
Contributor

Version 3.1.6 will fix this issue. #13878

Does the UI also need to be fixed?

The ui part has not been fixed yet, but it has little impact. It would be even better if some front-end students are willing to fix it.

@zhuangchong zhuangchong modified the milestones: 3.1.6, 3.2.0 May 8, 2023
@zhuangchong zhuangchong removed the 3.1.x for 3.1.x version label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.0 for 3.2.0 version backend bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants