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-3900][server] kill multi yarn app in one job #4042

Merged
merged 11 commits into from
Nov 22, 2020
Merged

[FIX-3900][server] kill multi yarn app in one job #4042

merged 11 commits into from
Nov 22, 2020

Conversation

Eights-Li
Copy link
Contributor

What is the purpose of the pull request

fix #3900 kill multi yarn app in one job

Brief change log

  • modify server/utils/ProcessUtils.java cancelApplication method

Verify this pull request

  • server/utils/ProcessUtilsTest to verify this pr

Copy link
Contributor

@lenboo lenboo left a comment

Choose a reason for hiding this comment

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

some code smell can be wiped out.

Copy link
Contributor

@lenboo lenboo left a comment

Choose a reason for hiding this comment

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

two magic number.

@codecov-io
Copy link

codecov-io commented Nov 16, 2020

Codecov Report

Merging #4042 (802de19) into dev (f152cae) will increase coverage by 0.10%.
The diff coverage is 51.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #4042      +/-   ##
============================================
+ Coverage     39.39%   39.49%   +0.10%     
- Complexity     2968     2981      +13     
============================================
  Files           467      467              
  Lines         22104    22113       +9     
  Branches       2712     2714       +2     
============================================
+ Hits           8708     8734      +26     
+ Misses        12589    12570      -19     
- Partials        807      809       +2     
Impacted Files Coverage Δ Complexity Δ
...he/dolphinscheduler/server/utils/ProcessUtils.java 48.34% <48.29%> (+29.33%) 25.00 <25.00> (+17.00)
.../org/apache/dolphinscheduler/common/Constants.java 85.00% <100.00%> (ø) 1.00 <0.00> (ø)
...che/dolphinscheduler/common/utils/LoggerUtils.java 62.50% <0.00%> (-8.34%) 5.00% <0.00%> (-1.00%)
...e/dolphinscheduler/common/shell/AbstractShell.java 44.53% <0.00%> (-5.89%) 5.00% <0.00%> (-1.00%)
...er/master/dispatch/host/assign/RandomSelector.java 77.77% <0.00%> (-5.56%) 3.00% <0.00%> (-1.00%)
...inscheduler/service/zk/CuratorZookeeperClient.java 56.09% <0.00%> (-4.88%) 6.00% <0.00%> (-1.00%)
...pache/dolphinscheduler/common/utils/FileUtils.java 28.92% <0.00%> (-3.31%) 9.00% <0.00%> (ø%)
.../apache/dolphinscheduler/common/utils/OSUtils.java 44.57% <0.00%> (-2.41%) 23.00% <0.00%> (ø%)

Continue to review full report at Codecov.

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

FileUtils.writeStringToFile(new File(commandFile), sb.toString(), StandardCharsets.UTF_8);
}

String runCmd = "sh " + commandFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this "sh " can be instead?
I can find it elsewhere.

mat = MACPATTERN.matcher(pids);
}
} else {
String pids = OSUtils.exeCmd("pstree -p " + processId);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this "pstree " can be extracted to one variable, then I can easily replace this variable.

Copy link
Contributor

@lenboo lenboo left a comment

Choose a reason for hiding this comment

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

other magic numbers exists.

Copy link
Contributor

@lenboo lenboo left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@lgcareer lgcareer left a comment

Choose a reason for hiding this comment

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

+1

}

logger.info("kill cmd:{}", runCmd);
Runtime.getRuntime().exec(runCmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

please use OSUtils.exeCmd method instead,thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fixed

Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

don't use Runtime.getRuntime().exec directly, replace with OSUtils.exeCmd, thx

@sonarcloud
Copy link

sonarcloud bot commented Nov 20, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

62.7% 62.7% Coverage
0.0% 0.0% Duplication

@Eights-Li
Copy link
Contributor Author

don't use Runtime.getRuntime().exec directly, replace with OSUtils.exeCmd, thx
please review this pr~

Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

+1
good job

@davidzollo davidzollo added this to the 1.3.4-release milestone Nov 22, 2020
@davidzollo davidzollo merged commit 368d3e8 into apache:dev Nov 22, 2020
@Eights-Li Eights-Li deleted the dev-kill-yarn-app branch November 25, 2020 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Kill YarnJob may be fail in cluster?
5 participants