-
Notifications
You must be signed in to change notification settings - Fork 314
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
LIVY-359. Cache livy logs as config driven #332
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -380,17 +380,9 @@ class InteractiveSession( | |
heartbeat() | ||
|
||
private val app = mockApp.orElse { | ||
if (livyConf.isRunningOnYarn()) { | ||
val driverProcess = client.flatMap { c => Option(c.getDriverProcess) } | ||
.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))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jerryshao Earlier we were returning None if the master is local (not yarn) so the driver logs didn't go to livy logs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's what I want. |
||
Option(SparkApp.create(appTag, appId, driverProcess, livyConf, Some(this))) | ||
} else { | ||
// When Livy is running with other cluster manager, SparkApp doesn't provide any | ||
// additional benefit over controlling RSCDriver using RSCClient. Don't use it. | ||
None | ||
} | ||
} | ||
|
||
if (client.isEmpty) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,13 @@ import java.util.concurrent.locks.ReentrantLock | |
|
||
import scala.io.Source | ||
|
||
import com.google.common.collect.EvictingQueue | ||
|
||
import com.cloudera.livy.Logging | ||
|
||
class LineBufferedStream(inputStream: InputStream) extends Logging { | ||
class LineBufferedStream(inputStream: InputStream, logSize: Int) extends Logging { | ||
|
||
private[this] var _lines: IndexedSeq[String] = IndexedSeq() | ||
private[this] val _lines: EvictingQueue[String] = EvictingQueue.create[String](logSize) | ||
|
||
private[this] val _lock = new ReentrantLock() | ||
private[this] val _condition = _lock.newCondition() | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: remove unnecessary white space. |
||
_lock.lock() | ||
try { | ||
_lines = _lines :+ line | ||
info(s"stdout: $line") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can move this line out of |
||
if(logSize > 0) _lines add line | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to add |
||
_condition.signalAll() | ||
} finally { | ||
_lock.unlock() | ||
} | ||
} | ||
|
||
_lines.map { line => info("stdout: ", line) } | ||
_lock.lock() | ||
try { | ||
_finished = true | ||
|
@@ -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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will introduce threading issue when copying to new array. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we add lock here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. |
||
|
||
def iterator: Iterator[String] = { | ||
new LinesIterator | ||
|
@@ -71,7 +73,7 @@ class LineBufferedStream(inputStream: InputStream) extends Logging { | |
private[this] var index = 0 | ||
|
||
override def hasNext: Boolean = { | ||
if (index < _lines.length) { | ||
if (_lines.size > 0) { | ||
true | ||
} else { | ||
// Otherwise we might still have more data. | ||
|
@@ -81,7 +83,7 @@ class LineBufferedStream(inputStream: InputStream) extends Logging { | |
false | ||
} else { | ||
_condition.await() | ||
index < _lines.length | ||
_lines.size > 0 | ||
} | ||
} finally { | ||
_lock.unlock() | ||
|
@@ -90,7 +92,7 @@ class LineBufferedStream(inputStream: InputStream) extends Logging { | |
} | ||
|
||
override def next(): String = { | ||
val line = _lines(index) | ||
val line = _lines.poll() | ||
index += 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Also I was thinking this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm. It's not thread safe. Added lock to it. |
||
line | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using configuration instead of hard-coded number. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it runs only when livy server is started. Do we need to make it config driven? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see. |
||
val exitCode = process.waitFor() | ||
val output = process.inputIterator.mkString("\n") | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,8 +40,8 @@ class SparkProcApp ( | |
} | ||
} | ||
|
||
override def log(): IndexedSeq[String] = process.inputLines | ||
|
||
override def log(): IndexedSeq[String] = | ||
("stdout: " +: process.inputLines) ++ ("\nstderr: " +: process.errorLines) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add empty line after it. |
||
private def changeState(newState: SparkApp.State.Value) = { | ||
if (state != newState) { | ||
listener.foreach(_.stateChanged(state, newState)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to rename this to "livy.session.cache-log.size".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Let me do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we name it like
livy.cache-log.size
or something as it applies to both interactive and batch?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'm fine with it.