[LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn #242

runzhiwang · 2019-10-06T04:16:58Z

What changes were proposed in this pull request?

Instead of spawning a monitor thread for every session, create a centralized thread to monitor all apps.

How was this patch tested?

Existed ITs and UTs.

codecov-io · 2019-10-06T05:02:30Z

Codecov Report

Merging #242 into master will increase coverage by 0.14%.
The diff coverage is 77.14%.

@@             Coverage Diff              @@
##             master     #242      +/-   ##
============================================
+ Coverage     68.09%   68.23%   +0.14%     
- Complexity      939      949      +10     
============================================
  Files           102      102              
  Lines          5883     5935      +52     
  Branches        893      904      +11     
============================================
+ Hits           4006     4050      +44     
- Misses         1302     1310       +8     
  Partials        575      575

Impacted Files	Coverage Δ	Complexity Δ
...rver/src/main/scala/org/apache/livy/LivyConf.scala	`96.07% <100%> (+0.05%)`	`21 <0> (ø)`	⬇️
...ain/scala/org/apache/livy/utils/SparkYarnApp.scala	`76.07% <76.47%> (+1.07%)`	`50 <7> (+10)`	⬆️
...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala	`52.94% <0%> (-2.95%)`	`7% <0%> (ø)`
...c/src/main/java/org/apache/livy/rsc/RSCClient.java	`73.49% <0%> (+1.2%)`	`20% <0%> (ø)`	⬇️
...ala/org/apache/livy/scalaapi/LivyScalaClient.scala	`86.66% <0%> (+3.33%)`	`7% <0%> (ø)`	⬇️

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 319386a...3144ff1. Read the comment docs.

conf/livy.conf.template

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

jahstreet

Nice PR proposal, it should have been done long time ago) Added some comments. I see that improvements that can be done here with rearranging code a bit. Would you mind if I propose some changes on to your branch?
Suggestions: runzhiwang#1

conf/livy.conf.template

server/src/main/scala/org/apache/livy/LivyConf.scala

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

runzhiwang · 2019-11-08T11:02:25Z

@jahstreet Thank you for your comments. I have updated the PR.

jahstreet · 2019-11-08T11:10:13Z

@runzhiwang Yeah, I see, will go through once again during weekend. Good job man!

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

jerryshao · 2019-11-14T02:25:42Z

I still need a close look to see the detailed implementations.

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

jerryshao · 2019-11-20T02:42:18Z

I'm OK with the current changes, @yiheng would you please also take a look.

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

jerryshao · 2019-11-22T03:06:16Z

Looks ok from my side, @yiheng please help to review again.

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

server/src/main/scala/org/apache/livy/LivyConf.scala

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

server/src/main/scala/org/apache/livy/LivyConf.scala

jahstreet

Added some comments, other than that looks nice so far, good job!

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

runzhiwang · 2019-12-06T11:47:52Z

@jahstreet Thank you very much, I have updated according to your comments.

jahstreet · 2019-12-06T12:01:55Z

@runzhiwang can you test your patch on a Yarn cluster?

runzhiwang · 2019-12-06T12:02:53Z

@jahstreet OK.

runzhiwang · 2019-12-07T08:27:41Z

@jahstreet Hi, I have test the patch, and it works fine.

yiheng · 2019-12-11T07:57:41Z

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala

+          Thread.sleep(yarnPoolInterval)
+        } catch {
+          case e: InterruptedException =>
+            loop = false


Should we keep in the loop? I'm afraid the thread is interrupted while the livy is still running. Then all the app status won't change.

sanchay0 · 2020-04-16T01:37:25Z

Hey @runzhiwang what's the status of this PR? Are you going to merge this in? I'm having issues with dangling threads with a lot of jobs running simultaneously and this fix will be great to have!

runzhiwang · 2020-04-16T01:50:51Z

@sanchay0 Thanks for your attention. I have completed the code. And It just need more review.

codecov-commenter · 2024-11-18T08:39:45Z

Codecov Report

Attention: Patch coverage is 77.14286% with 24 lines in your changes missing coverage. Please review.

Project coverage is 68.23%. Comparing base (0a527e8) to head (3144ff1).
Report is 64 commits behind head on master.

Files with missing lines	Patch %	Lines
...ain/scala/org/apache/livy/utils/SparkYarnApp.scala	76.47%	17 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #242   +/-   ##
=========================================
  Coverage     68.23%   68.23%           
- Complexity      941      949    +8     
=========================================
  Files           102      102           
  Lines          5883     5935   +52     
  Branches        893      904   +11     
=========================================
+ Hits           4014     4050   +36     
- Misses         1295     1310   +15     
- Partials        574      575    +1

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

jerryshao reviewed Oct 10, 2019

View reviewed changes

conf/livy.conf.template Outdated Show resolved Hide resolved

jerryshao reviewed Oct 10, 2019

View reviewed changes

jahstreet reviewed Nov 3, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

jahstreet mentioned this pull request Nov 3, 2019

Suggestions to LIVY-336 runzhiwang/incubator-livy#1

Closed

runzhiwang force-pushed the merge-monitor-thread branch 2 times, most recently from 8839fb4 to f949f66 Compare November 8, 2019 09:30

runzhiwang closed this Nov 12, 2019

runzhiwang reopened this Nov 12, 2019

runzhiwang force-pushed the merge-monitor-thread branch 2 times, most recently from 01a445e to 2b38d51 Compare November 12, 2019 09:17

jerryshao reviewed Nov 14, 2019

View reviewed changes

runzhiwang changed the title ~~[LIVY-336] Livy should not spawn one thread per job to track the job on Yarn~~ [LIVY-336][WIP] Livy should not spawn one thread per job to track the job on Yarn Nov 19, 2019

jerryshao reviewed Nov 20, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

jerryshao reviewed Nov 20, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

yiheng reviewed Nov 20, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

runzhiwang force-pushed the merge-monitor-thread branch 5 times, most recently from ce12169 to 61d5d38 Compare November 22, 2019 00:43

runzhiwang changed the title ~~[LIVY-336][WIP] Livy should not spawn one thread per job to track the job on Yarn~~ [LIVY-336] Livy should not spawn one thread per job to track the job on Yarn Nov 22, 2019

runzhiwang changed the title ~~[LIVY-336] Livy should not spawn one thread per job to track the job on Yarn~~ [LIVY-336][Server] Livy should not spawn one thread per job to track the job on Yarn Nov 22, 2019

runzhiwang changed the title ~~[LIVY-336][Server] Livy should not spawn one thread per job to track the job on Yarn~~ [LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn Nov 22, 2019

runzhiwang added 4 commits November 26, 2019 20:16

[LIVY-336] fix code review

8b82288

[LIVY-336] fix code review

e8f83e2

[LIVY-336] remove redundant code

0584a8a

[LIVY-336] fix code review

9521863

runzhiwang force-pushed the merge-monitor-thread branch from d5d0b0a to 9521863 Compare November 26, 2019 12:23

yiheng reviewed Nov 27, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

yiheng reviewed Nov 27, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

runzhiwang force-pushed the merge-monitor-thread branch 3 times, most recently from 12ebee8 to b18fad1 Compare November 29, 2019 07:19

[LIVY-336] add a thread to check whether the monitor was blocked

b21e404

runzhiwang force-pushed the merge-monitor-thread branch from b18fad1 to b21e404 Compare November 29, 2019 12:07

jerryshao reviewed Dec 3, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/LivyConf.scala Outdated Show resolved Hide resolved

jerryshao reviewed Dec 3, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/LivyConf.scala Show resolved Hide resolved

jerryshao reviewed Dec 3, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

remove redundant config and break while when interrupt

fc698c9

jerryshao reviewed Dec 6, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/LivyConf.scala Show resolved Hide resolved

Fix error default value in config

c3c86e3

runzhiwang force-pushed the merge-monitor-thread branch from 24561ab to c3c86e3 Compare December 6, 2019 07:01

jahstreet reviewed Dec 6, 2019

View reviewed changes

server/src/main/scala/org/apache/livy/utils/SparkYarnApp.scala Outdated Show resolved Hide resolved

fix code review

3144ff1

yiheng reviewed Dec 11, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn #242

[LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn #242

runzhiwang commented Oct 6, 2019

codecov-io commented Oct 6, 2019 •

edited

Loading

jahstreet left a comment •

edited

Loading

runzhiwang commented Nov 8, 2019

jahstreet commented Nov 8, 2019

jerryshao commented Nov 14, 2019

jerryshao commented Nov 20, 2019

jerryshao commented Nov 22, 2019

jahstreet left a comment

runzhiwang commented Dec 6, 2019

jahstreet commented Dec 6, 2019

runzhiwang commented Dec 6, 2019

runzhiwang commented Dec 7, 2019

yiheng Dec 11, 2019

sanchay0 commented Apr 16, 2020

runzhiwang commented Apr 16, 2020 •

edited

Loading

codecov-commenter commented Nov 18, 2024

[LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn #242

Are you sure you want to change the base?

[LIVY-336][SERVER] Livy should not spawn one thread per job to track the job on Yarn #242

Conversation

runzhiwang commented Oct 6, 2019

What changes were proposed in this pull request?

How was this patch tested?

codecov-io commented Oct 6, 2019 • edited Loading

Codecov Report

jahstreet left a comment • edited Loading

Choose a reason for hiding this comment

runzhiwang commented Nov 8, 2019

jahstreet commented Nov 8, 2019

jerryshao commented Nov 14, 2019

jerryshao commented Nov 20, 2019

jerryshao commented Nov 22, 2019

jahstreet left a comment

Choose a reason for hiding this comment

runzhiwang commented Dec 6, 2019

jahstreet commented Dec 6, 2019

runzhiwang commented Dec 6, 2019

runzhiwang commented Dec 7, 2019

yiheng Dec 11, 2019

Choose a reason for hiding this comment

sanchay0 commented Apr 16, 2020

runzhiwang commented Apr 16, 2020 • edited Loading

codecov-commenter commented Nov 18, 2024

Codecov Report

codecov-io commented Oct 6, 2019 •

edited

Loading

jahstreet left a comment •

edited

Loading

runzhiwang commented Apr 16, 2020 •

edited

Loading