[WIP]: [Livy-336]: Livy should not spawn one thread per job to track the job on Yarn (Use YARN REST API) by meisam · Pull Request #44 · apache/livy

meisam · 2017-09-06T18:41:08Z

What changes were proposed in this pull request?

Currently, Livy spawns one thread to for each Spark application to track the status of the application on YARN. The threads are called yarnAppMonitorThread-xxxx and make multiple calls to YARN using YARN's Java API to get the status of running applications or to kill applications. Spawning so many threads is a bottle neck and limits the number of Spark applications that can be submitted in a short span of time.

This pull request addresses the above issue in 2 ways:

It creates only one thread to poll YARN and get the status of all Spark applications.
It replaces the Java API with YARN REST API to eliminate multiple calls to YARN.

How was this patch tested?

The existing unit tests and integration test are updated to reflect the changes in the code.
Every iteration of this PR is tested internally at PayPal at dev and production development. Some iterations have been running in productions for months.

What is not in the scope of this pull request?

Based on the discussion and feedback on this pull request, and based on the feedback

Calls to yarnClient.kill wont be replaced with REST API to kill YARN apps because:
- The current kill REST API is experimental.
Maven dependencies to YARN won't be removed because:
- There is minimal benefit in removing all YARN dependencies. Livy still depends on Hadoop libraries.
- If we remove YARN dependencies, we should find a different way to get the Name Node URL. One way to do that is to add new parameters in `livy.con, but that adds complexity and possibly confusion.

Task-url: https://issues.apache.org/jira/browse/LIVY-336

Yarn * Moved YARN related code from `SparkYarnApp` to a new class `YarnInterface` * Removed "yarnAppMinotorThreads" that are created for each Spark application * Created one thread that polls YARN to get `ApplicationId`s and * Created a pool of threads that gets `ApplicationAttempts` and `ContainerReports` from YARN * because there is no API to get `ApplicationAttempts` and `ContainerReports` in bulk * the thread pool is actually used by Scala `Future`s * Updated test cases in `SparkYarnAppSpec` to match the new design Task-Url: https://issues.cloudera.org/browse/LIVY-336

YARN interface useses REST API now.

vanzin · 2017-09-06T18:48:59Z

we can drop all maven dependencies to YARN

Not sure that brings a lot of gains; unless something changed, Livy uses a bunch of HDFS APIs already, so removing YARN would probably not remove a whole lot of dependencies.

meisam · 2017-09-06T18:56:47Z

Not sure that brings a lot of gains; unless something changed, Livy uses a bunch of HDFS APIs already, so removing YARN would probably not remove a whole lot of dependencies.

On top of that, if we remove all dependencies to YARN, we have to put the YARN cofings, such as resouce manager REST URL, in livy.conf. If we keep the dependencies, we can read the configs from yarn-site.xml and build the REST URL.

codecov-io · 2017-09-06T21:33:49Z

Codecov Report

Merging #44 into master will decrease coverage by 4.2%.
The diff coverage is 47.54%.

@@             Coverage Diff              @@
##             master      #44      +/-   ##
============================================
- Coverage     70.73%   66.52%   -4.21%     
+ Complexity      788      787       -1     
============================================
  Files            97       99       +2     
  Lines          5388     5556     +168     
  Branches        802      850      +48     
============================================
- Hits           3811     3696     -115     
- Misses         1037     1323     +286     
+ Partials        540      537       -3

Impacted Files	Coverage Δ	Complexity Δ
...main/scala/org/apache/livy/server/LivyServer.scala	`1.75% <0%> (-35.31%)`	`2 <0> (-8)`
...rver/src/main/scala/org/apache/livy/LivyConf.scala	`92.96% <100%> (-2.97%)`	`12 <0> (-3)`
...rc/main/scala/org/apache/livy/utils/SparkApp.scala	`53.12% <37.5%> (-27.65%)`	`1 <0> (ø)`
...in/scala/org/apache/livy/utils/YarnInterface.scala	`42.23% <42.23%> (ø)`	`20 <20> (?)`
...ain/scala/org/apache/livy/utils/SparkYarnApp.scala	`30.26% <59.25%> (-31.71%)`	`33 <22> (+4)`
...cala/org/apache/livy/utils/ApplicationReport.scala	`93.75% <93.75%> (ø)`	`1 <1> (?)`
...r/src/main/scala/org/apache/livy/utils/Clock.scala	`42.85% <0%> (-57.15%)`	`0% <0%> (ø)`
core/src/main/scala/org/apache/livy/Logging.scala	`54.54% <0%> (-18.19%)`	`0% <0%> (ø)`
... and 25 more

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 05bfa15...1555157. Read the comment docs.

jerryshao · 2017-09-08T08:19:23Z

What's the benefit of using REST API?

meisam · 2017-09-08T16:51:53Z

The main benefit is that with one call we can get all the info we need for the app, that is appId, state, finalStatus, diagnostics, trackingUrl, SparkUiUrl, etc.
With Yarn's Java API, we can get appId with one call, but to get trakingURL, SparkUiUrl, etc., we have to make extra calls. First we need to get the attemptId, then we need to get the containerId, and then we can get the remaining information.

There is one other potential benefit, which is we can drop dependencies to YARN. But, as Marcelo described, the gains from dropping YARN dependencies are minimal.

I am alright with either YARN's Java API or REST API. I just added this PR because the proposal on the mailing list recommended REST.

ajbozarth

Gave feedback where I found it, but I'm no YARN expect so my review of the new YarnInterface class wasn't detailed. I also haven't had a chance to read through the Spec changes.

ajbozarth · 2017-09-15T00:25:37Z

+  val YARN_REST_HOST = Entry("livy.server.yarn.rest.host", "localhost")
+
+  // The resource manager port for YARN's REST API.
+  val YARN_REST_PORT = Entry("livy.server.yarn.rest.port", 0)


I'm pretty sure a default port of 0 is a bad idea here

I'll change it the the default port 8088

ajbozarth · 2017-09-15T00:26:35Z

+  // The resource manager port for YARN's REST API.
+  val YARN_REST_PORT = Entry("livy.server.yarn.rest.port", 0)
+
+  // The resource manager port for YARN's REST API.


comment for wrong val

ajbozarth · 2017-09-15T00:27:13Z

+  val YARN_REST_PORT = Entry("livy.server.yarn.rest.port", 0)
+
+  // The resource manager port for YARN's REST API.
+  val YARN_REST_PATH = Entry("livy.server.yarn.rest.path", "/ws/v1/cluster/apps")


why /ws/v1/cluster/apps? I'm not sure of the purpose of that particular path

It is just the default path. See https://hadoop.apache.org/docs/stable/hadoop-yarn/hadoop-yarn-site/ResourceManagerRest.html#Cluster_Information_API

ajbozarth · 2017-09-15T21:18:11Z

+                              state: String,
+                              trackingUI: Option[String],
+                              trackingUrl: Option[String]
+                            )


Two issues with this whole declaration:

Lots of commented out params, why?

I'm pretty sure this is bad style, try running style check and looking at other similar declarations

This is one thing that I wanted to discuss with the Livy community. Scala cannot have case classes with more than 22 fields. I put all the fields that we can get from YARN REST for reference, but I commented out ones that are not as useful for Livy. I'd like to get feedback from the Livy users on which fields we should keep and which one we should leave out.

That makes sense, @jerryshao @zjffdu you guys probably know this side of Livy better, what fields do you think are best to include?

@meisam I thought that only applied to case classes in 2.10; so you could work around it by not having a case class or by using 2.11 (which is what Livy uses, no?).

Livy itself is still built with scala 2.10, there's an open JIRA for the change that I tried to get traction on a while back with no response.

ajbozarth · 2017-09-15T21:31:43Z

+        SparkApp.State.KILLED
+      case (state, finalStatus) => // any other combination is invalid
+        error(s"Unknown YARN state $state for app $appId with final status $finalStatus.")
+        SparkApp.State.FAILED


Are the above changes just #39 pulled over so this change works?

Yes, except for one thing. #39 uses Java objects and enums. With YARN REST API, these are string values.

Ok so this is a separate but related/dependent change then?

That is right. This is a separate but related change.

ajbozarth · 2017-09-15T21:32:30Z

-  import SparkYarnApp._

-  private val appIdPromise: Promise[ApplicationId] = Promise()
+  import scala.concurrent.ExecutionContext.Implicits.global


What is the purpose of this import?

It is needed as an implicit parameter to scala Future.

It is? I've never noticed it in other classes that use scala Future

ajbozarth · 2017-09-15T21:33:26Z

+  import scala.concurrent.ExecutionContext.Implicits.global
+
+  val appId: Future[ApplicationId] = Future {
+    appIdOption.fold(yarnInterface.getAppIdFromTag(appTag, process))(ConverterUtils.toApplicationId)


This seems messy but personally I'm not sure how exactly to clean it up

I took multiple shots at this, and this is the best that I could come up with. More feedback is appreciated.

I'll trust you on that and say this is ok then

ajbozarth · 2017-09-15T21:34:57Z

+class YarnInterface(livyConf: LivyConf, yarnClient: YarnClient, httpClient: HttpClient)
+  extends Logging {
+
+  import scala.concurrent.ExecutionContext.Implicits.global


again, purpose?

ajbozarth · 2017-09-15T21:35:43Z

+//
+//  def error(s: String) = {
+//    println(s)
+//  }


these debugs should be removed

ajbozarth · 2017-09-15T21:36:48Z

+    val limit = livyConf.getInt(LivyConf.YARN_REST_REPORTS_LIMIT)
+    val query = s"applicationTypes=$applicationTypes&limit=$limit"
+
+    // parameters URI(scheme, user, host, port, path, query, fragment)


purpose? remove?

Just to clarify null parameters on the next line. The URI class has too many constructors.

I think it's ok without, the URI class is a pretty standard class

meisam · 2017-09-18T20:51:29Z

Thanks @ajbozarth. This was helpful feedback.

jerryshao · 2017-09-19T03:06:36Z

I don't have a strong preference on changing to REST APIs, except one advantage you mentioned above (get all infos in one request). I'm not sure usually how many application will we launch in one Livy server, if it is only tens or hundreds of application, I think the communication overhead should not be big.

Also avoiding yarn dependency is not a big problem, anyway we still need to depend on hadoop jars.

One difference I can think of is to visit security Hadoop, now because we change to REST API, so we should provide spnego principal/keytab, rather than Livy principal/keytab.

meisam · 2017-09-19T12:49:47Z

I don't have a strong preference on changing to REST APIs, except one advantage you mentioned above (get all infos in one request).

The main gain from this PR is LIVY-336: Livy should not spawn one thread per job to track the job on Yarn.

One difference I can think of is to visit security Hadoop, now because we change to REST API, so we should provide spnego principal/keytab, rather than Livy principal/keytab.

YARN's Cluster Applications API does not need authentication with one exception: deleting a YARN app needs authentication. But the delete API is experimental and I am not sure if it is a good idea to use it.

Since dropping the YARN dependencies has minimal benefits, it makes sense to kill YARN apps with java

github-actions · 2026-04-28T00:56:36Z

This pull request has been automatically marked as stale because it has had no activity for at least 3 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Livy!

github-actions · 2026-07-06T00:55:14Z

Closing this pull request due to at least 4 months of inactivity. If you would like to continue the work, please feel free to reopen this pull request or open a new one.

meisam and others added 2 commits September 6, 2017 09:14

[LIVY-336] [WIP] Refactoring YarnInterface

1555157

YARN interface useses REST API now.

meisam changed the title ~~[WIP]: Livy 336: Replacing YARN interface with YARN REST API~~ [WIP]: [Livy-336]: Replacing YARN interface with YARN REST API Sep 6, 2017

meisam mentioned this pull request Sep 8, 2017

[LIVY-396] Livy does not map YARN app states correctly #39

Closed

ajbozarth reviewed Sep 15, 2017

View reviewed changes

meisam changed the title ~~[WIP]: [Livy-336]: Replacing YARN interface with YARN REST API~~ [WIP]: [Livy-336]: Livy should not spawn one thread per job to track the job on Yarn Oct 17, 2017

meisam changed the title ~~[WIP]: [Livy-336]: Livy should not spawn one thread per job to track the job on Yarn~~ [WIP]: [Livy-336]: Livy should not spawn one thread per job to track the job on Yarn (Use YARN REST API) Oct 17, 2017

github-actions Bot added the stale label Apr 28, 2026

github-actions Bot closed this Jul 6, 2026

Uh oh!

Conversation

meisam commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

What is not in the scope of this pull request?

Uh oh!

vanzin commented Sep 6, 2017

Uh oh!

meisam commented Sep 6, 2017

Uh oh!

codecov-io commented Sep 6, 2017

Codecov Report

Uh oh!

jerryshao commented Sep 8, 2017

Uh oh!

meisam commented Sep 8, 2017

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisam commented Sep 18, 2017

Uh oh!

jerryshao commented Sep 19, 2017

Uh oh!

meisam commented Sep 19, 2017

Uh oh!

github-actions Bot commented Apr 28, 2026

Uh oh!

meisam commented Sep 6, 2017 •

edited

Loading