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-257. access log is not available #239
Conversation
LGTM, the test failure should also be fixed. |
@@ -693,6 +695,7 @@ | |||
<environmentVariables> | |||
<LIVY_TEST>true</LIVY_TEST> | |||
<SPARK_HOME>${spark.home}</SPARK_HOME> | |||
<LIVY_LOG_DIR>${livy.log.dir}</LIVY_LOG_DIR> |
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.
Will this change the log output dir for integration test, previously it is under miniLivyMain
.
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.
It is still under miniLivyMain
, but the access log is under target
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 see, thanks.
Current coverage is 71.92% (diff: 100%)@@ master #239 diff @@
==========================================
Files 90 90
Lines 4539 4541 +2
Methods 0 0
Messages 0 0
Branches 772 772
==========================================
+ Hits 3262 3266 +4
+ Misses 828 826 -2
Partials 449 449
|
@@ -71,6 +71,8 @@ | |||
<test.redirectToFile>true</test.redirectToFile> | |||
<execution.root>${user.dir}</execution.root> | |||
<spark.home>${execution.root}/dev/spark</spark.home> | |||
<!-- used for testing, NCSARequestLog use it for access log --> | |||
<livy.log.dir>${basedir}/target</livy.log.dir> |
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.
Nit: Mind if we keep it under MiniLivyMain for integration test?
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.
The reason I put it in parent is that HttpClientSpec
also use WebServer.
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 see.
@@ -74,12 +72,14 @@ class WebServer(livyConf: LivyConf, var host: String, var port: Int) extends Log | |||
val handlers = new HandlerCollection | |||
handlers.addHandler(context) | |||
|
|||
// configure the access log | |||
// configure the access log |
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.
Nit:
// Configure the access log
val requestLogHandler = new RequestLogHandler | ||
val requestLog = new NCSARequestLog("/jetty-yyyy_mm_dd.request.log") | ||
val requestLog = new NCSARequestLog(sys.env.getOrElse("LIVY_LOG_DIR", | ||
sys.env("LIVY_HOME") + "/logs") + "/jetty-yyyy_mm_dd.request.log") |
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.
Nit: name it "yyyy_mm_dd.request.log" instead of "jetty-yyyy_mm_dd.request.log"
LGTM. |
Straightforward bugfix for access log file.