LIVY-359. Cache livy logs as config driven #332

praveen-kanamarlapudi · 2017-05-15T21:29:00Z

Changes Made:

Added logs cacheing as config driven.
Fixed adding driver process logs livy logs for local and client mode in interactive mode.

praveen-kanamarlapudi · 2017-05-16T00:15:15Z

server/src/main/scala/com/cloudera/livy/server/interactive/InteractiveSession.scala

-        .map(new LineBufferedProcess(_))
-      // When Livy is running with YARN, SparkYarnApp can provide better YARN integration.
-      // (e.g. Reflect YARN application state to session state).
+        .map(new LineBufferedProcess(_, livyConf.getInt(LivyConf.SPARK_LOGS_SIZE)))


@jerryshao Earlier we were returning None if the master is local (not yarn) so the driver logs didn't go to livy logs.
I think this change should solve the problem.
Let me know if we are ok with this change?

Yes, that's what I want.

praveen-kanamarlapudi · 2017-05-16T00:19:21Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

        _lock.lock()
        try {
-          _lines = _lines :+ line
+          info(s"stdout: $line")


@jerryshao In client mode we were seeing spark driver logs in livy log after great delay as the log statement was after the loop. I agree log inside the lock is heavy.

Let me know if there is a better way to handle the scenario.

I think we can move this line out of try finally block.

codecov-io · 2017-05-16T00:21:51Z

Codecov Report

Merging #332 into master will increase coverage by 0.06%.
The diff coverage is 76.19%.

@@             Coverage Diff              @@
##             master     #332      +/-   ##
============================================
+ Coverage     70.42%   70.49%   +0.06%     
- Complexity      726      727       +1     
============================================
  Files            96       96              
  Lines          5123     5124       +1     
  Branches        774      774              
============================================
+ Hits           3608     3612       +4     
+ Misses          997      994       -3     
  Partials        518      518

Impacted Files	Coverage Δ	Complexity Δ
...scala/com/cloudera/livy/utils/LivySparkUtils.scala	`69.86% <100%> (ø)`	`0 <0> (ø)`	⬇️
.../com/cloudera/livy/utils/SparkProcessBuilder.scala	`57.14% <100%> (ø)`	`12 <0> (ø)`	⬇️
...n/scala/com/cloudera/livy/utils/SparkProcApp.scala	`41.17% <100%> (+2.28%)`	`5 <1> (ø)`	⬇️
...er/src/main/scala/com/cloudera/livy/LivyConf.scala	`94.4% <100%> (+0.04%)`	`16 <0> (ø)`	⬇️
...a/livy/server/interactive/InteractiveSession.scala	`63.05% <33.33%> (-0.87%)`	`42 <1> (ø)`
.../com/cloudera/livy/utils/LineBufferedProcess.scala	`73.33% <66.66%> (ø)`	`7 <1> (ø)`	⬇️
...a/com/cloudera/livy/utils/LineBufferedStream.scala	`82.5% <81.81%> (+4.12%)`	`5 <2> (ø)`	⬇️
...la/com/cloudera/livy/scalaapi/ScalaJobHandle.scala	`55.88% <0%> (+2.94%)`	`0% <0%> (ø)`	⬇️
...va/com/cloudera/livy/rsc/rpc/KryoMessageCodec.java	`98.11% <0%> (+3.77%)`	`19% <0%> (+1%)`	⬆️

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 59af39d...2d092ae. Read the comment docs.

jerryshao · 2017-05-16T06:27:28Z

@praveenkanamarlapudi thanks for bring this out, could you please file a JIRA and update the PR title first.

jerryshao · 2017-05-16T06:39:09Z

Also please fix the UTs.

jerryshao · 2017-05-16T07:36:10Z

server/src/main/scala/com/cloudera/livy/server/interactive/InteractiveSession.scala

-        .map(new LineBufferedProcess(_))
-      // When Livy is running with YARN, SparkYarnApp can provide better YARN integration.
-      // (e.g. Reflect YARN application state to session state).
+        .map(new LineBufferedProcess(_, livyConf.getInt(LivyConf.SPARK_LOGS_SIZE)))


Yes, that's what I want.

jerryshao · 2017-05-16T07:37:10Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

@@ -36,17 +38,17 @@ class LineBufferedStream(inputStream: InputStream) extends Logging {
  private val thread = new Thread {
    override def run() = {
      val lines = Source.fromInputStream(inputStream).getLines()
-      for (line <- lines) {
+       for (line <- lines) {


nit: remove unnecessary white space.

jerryshao · 2017-05-16T07:39:38Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

        _lock.lock()
        try {
-          _lines = _lines :+ line
+          info(s"stdout: $line")


I think we can move this line out of try finally block.

jerryshao · 2017-05-16T07:43:22Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

        _lock.lock()
        try {
-          _lines = _lines :+ line
+          info(s"stdout: $line")
+          if(logSize > 0) _lines add line


Do we need to add if check, I assume if logSize is zero then we cannot add line to this queue.

jerryshao · 2017-05-16T07:44:49Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

@@ -59,7 +61,7 @@ class LineBufferedStream(inputStream: InputStream) extends Logging {
  thread.setDaemon(true)
  thread.start()

-  def lines: IndexedSeq[String] = _lines
+  def lines: IndexedSeq[String] = IndexedSeq.empty[String] ++ _lines.toArray(Array.empty[String])


I think this will introduce threading issue when copying to new array.

Shall we add lock here?

I think so.

jerryshao · 2017-05-16T07:46:13Z

server/src/main/scala/com/cloudera/livy/utils/LivySparkUtils.scala

@@ -100,7 +100,7 @@ object LivySparkUtils extends Logging {
      pb.environment().put("LIVY_TEST_CLASSPATH", sys.props("java.class.path"))
    }

-    val process = new LineBufferedProcess(pb.start())
+    val process = new LineBufferedProcess(pb.start(), 200)


Using configuration instead of hard-coded number.

I think it runs only when livy server is started. Do we need to make it config driven?
If the number is set too low by mistake, the livy server mayn't start.

jerryshao · 2017-05-16T07:47:59Z

server/src/main/scala/com/cloudera/livy/utils/SparkApp.scala

@@ -92,7 +92,8 @@ object SparkApp {
    if (livyConf.isRunningOnYarn()) {
      new SparkYarnApp(uniqueAppTag, appId, process, listener, livyConf)
    } else {
-      require(process.isDefined, "process must not be None when Livy master is not YARN.")
+      // process is None in recovery mode
+      // require(process.isDefined, "process must not be None when Livy master is not YARN.")
      new SparkProcApp(process.get, listener)


If process can be None, will this process.get work?

jerryshao · 2017-05-16T07:48:44Z

server/src/main/scala/com/cloudera/livy/utils/SparkProcApp.scala

-  override def log(): IndexedSeq[String] = process.inputLines
-
+  override def log(): IndexedSeq[String] =
+    ("stdout: " +: process.inputLines) ++ ("\nstderr: " +: process.errorLines)


Add empty line after it.

jerryshao · 2017-05-17T01:38:30Z

conf/livy.conf.template

@@ -31,6 +31,9 @@
 # If livy should impersonate the requesting users when creating a new session.
 # livy.impersonation.enabled = true

+# Logs size livy can cache for each session/batch. 0 means don't cache the logs.
+# livy.spark.logs.size = 200 


I would suggest to rename this to "livy.session.cache-log.size".

Sure. Let me do that.

can we name it like livy.cache-log.size or something as it applies to both interactive and batch?

OK, I'm fine with it.

jerryshao · 2017-05-17T01:41:26Z

server/src/main/scala/com/cloudera/livy/server/interactive/InteractiveSession.scala

-    }
+    val driverProcess = client.flatMap { c => Option(c.getDriverProcess) }
+        .map(new LineBufferedProcess(_, livyConf.getInt(LivyConf.SPARK_LOGS_SIZE)))
+    driverProcess.map(_ => SparkApp.create(appTag, appId, driverProcess, livyConf, Some(this)))


This line is a bit strange and not so scala-ish. Why not keep the original style.

driverProcess can be None for session recovery. We need to return None for that case and create SparkApp otherwise. Let me know if there is a better way to do that.

I see. Thanks.

Also can we change the style to:

driverProcess.map { _ => SparkApp.create(appTag, appId, driverProcess, livyConf, Some(this)) }

jerryshao · 2017-05-17T01:45:17Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

-        try {
-          _lines = _lines :+ line
+         try {
+          _lines add line


Remove the prefix whitespace before try. Also be better to use _lines.add(line) for clearance.

My bad. let me update it.

praveen-kanamarlapudi · 2017-05-18T17:03:05Z

@jerryshao updated PR as per the comments. Please have a look at it.

jerryshao · 2017-05-19T03:06:45Z

server/src/main/scala/com/cloudera/livy/utils/LineBufferedStream.scala

@@ -90,7 +97,7 @@ class LineBufferedStream(inputStream: InputStream) extends Logging {
    }

    override def next(): String = {
-      val line = _lines(index)
+      val line = _lines.poll()
      index += 1


This index seems not used any more.

Also I was thinking this poll may has threading issue with add. I'm not sure if EvictingQueue is a thread safe queue.

Hm. It's not thread safe. Added lock to it.

jerryshao · 2017-05-22T08:18:30Z

LGTM, thanks for the fix.

praveen-kanamarlapudi · 2017-06-07T17:46:57Z

@jerryshao Are you waiting for any changes from me?

jerryshao · 2017-06-08T00:37:09Z

No, I don't have further comments, because I don't have merging permission, so it is pending.

CC @zjffdu can you please take a review, thanks!

praveen-kanamarlapudi · 2017-06-12T20:49:04Z

@alex-the-man @zjffdu can you please review the PR.

jerryshao · 2017-06-30T07:49:07Z

Hi @praveenkanamarlapudi sorry for delay, can you please submit this PR against incubator-livy (https://github.com/apache/incubator-livy/pulls)?

jerryshao · 2017-07-05T06:20:51Z

Hi @praveenkanamarlapudi would you please submit this PR again in incubator-livy? If you're not working on this, I can help to cherry-pick the changes.

praveen-kanamarlapudi · 2017-07-05T08:49:32Z

Hi @jerryshao I will make the PR tomorrow.

praveen-kanamarlapudi · 2017-07-07T02:27:40Z

Made the PR-14

praveen-kanamarlapudi commented May 16, 2017

View reviewed changes

Added cache log size as config driven.

e94f507

praveen-kanamarlapudi force-pushed the master branch from 7d76aae to e94f507 Compare May 16, 2017 00:21

jerryshao suggested changes May 16, 2017

View reviewed changes

praveen-kanamarlapudi changed the title ~~Added logs cacheing as config driven.~~ LIVY-359. Cache livy logs as config driven May 16, 2017

praveen-kanamarlapudi force-pushed the master branch 2 times, most recently from 6c9dba2 to 75d8511 Compare May 16, 2017 22:43

jerryshao reviewed May 17, 2017

View reviewed changes

praveen-kanamarlapudi force-pushed the master branch from 75d8511 to 2d092ae Compare May 17, 2017 09:31

jerryshao reviewed May 19, 2017

View reviewed changes

Updated code as per the comments

f84bf47

praveen-kanamarlapudi force-pushed the master branch from 2d092ae to f84bf47 Compare May 22, 2017 06:07

LIVY-359. Cache livy logs as config driven #332

Are you sure you want to change the base?

LIVY-359. Cache livy logs as config driven #332

Conversation

praveen-kanamarlapudi commented May 15, 2017

praveen-kanamarlapudi May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 16, 2017 • edited Loading

Codecov Report

jerryshao commented May 16, 2017 • edited Loading

jerryshao commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveen-kanamarlapudi May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveen-kanamarlapudi commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 22, 2017

praveen-kanamarlapudi commented Jun 7, 2017

jerryshao commented Jun 8, 2017

praveen-kanamarlapudi commented Jun 12, 2017

jerryshao commented Jun 30, 2017

jerryshao commented Jul 5, 2017

praveen-kanamarlapudi commented Jul 5, 2017

praveen-kanamarlapudi commented Jul 7, 2017

praveen-kanamarlapudi May 16, 2017 •

edited

Loading

codecov-io commented May 16, 2017 •

edited

Loading

jerryshao commented May 16, 2017 •

edited

Loading

praveen-kanamarlapudi May 16, 2017 •

edited

Loading