[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309980622
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
Thinking about the `livy.conf.template` some more, maybe we should still
keep it there? That is actually how I found out about the possibility of adding
a basePath. Perhaps we can just tweak it to the following:
# The base path the server should use. By default the server is mounted
on "/".
# E.g.: livy.server.basePath = /my_livy - results in mounting on
/my_livy/
# livy.server.basePath =
There are plenty of other options in that file with empty string as default
(e.g. `livy.spark.deploy-mode`) so having it like that would not make this
config option an odd one out of the pack.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] codecov-io commented on issue #193: [LIVY-621]add dynamic service discovry for thrift server
codecov-io commented on issue #193: [LIVY-621]add dynamic service discovry for thrift server URL: https://github.com/apache/incubator-livy/pull/193#issuecomment-517544120 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=h1) Report > Merging [#193](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/788767e4cced78f18798f73a651f3ec2d087f938?src=pr=desc) will **increase** coverage by `0.05%`. > The diff coverage is `100%`. [](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#193 +/- ## === + Coverage 68.54% 68.6% +0.05% Complexity 906 906 === Files 100 100 Lines 56745688 +14 Branches854 854 === + Hits 38893902 +13 - Misses 12281229 +1 Partials557 557 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.28%)` | `21 <0> (ø)` | :arrow_down: | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `77.96% <0%> (ø)` | `41% <0%> (ø)` | :arrow_down: | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=footer). Last update [788767e...09f41a8](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #193: [LIVY-621]add dynamic service discovry for thrift server
codecov-io edited a comment on issue #193: [LIVY-621]add dynamic service discovry for thrift server URL: https://github.com/apache/incubator-livy/pull/193#issuecomment-517544120 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=h1) Report > Merging [#193](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/788767e4cced78f18798f73a651f3ec2d087f938?src=pr=desc) will **increase** coverage by `0.05%`. > The diff coverage is `100%`. [](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=tree) ```diff @@ Coverage Diff @@ ## master#193 +/- ## === + Coverage 68.54% 68.6% +0.05% Complexity 906 906 === Files 100 100 Lines 56745688 +14 Branches854 854 === + Hits 38893902 +13 - Misses 12281229 +1 Partials557 557 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.13% <100%> (+0.28%)` | `21 <0> (ø)` | :arrow_down: | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/193/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `77.96% <0%> (ø)` | `41% <0%> (ø)` | :arrow_down: | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=footer). Last update [788767e...09f41a8](https://codecov.io/gh/apache/incubator-livy/pull/193?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309977304
##
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##
@@ -60,7 +60,7 @@ object LivyConf {
val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
- val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
+ val SERVER_BASE_PATH = Entry("livy.ui.basePath", "/")
Review comment:
I think I found it: `LivyConf.configsWithAlternatives`. I will add
`livy.ui.basePath` there, pointing it to the key `livy.server.basepath` instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309977304
##
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##
@@ -60,7 +60,7 @@ object LivyConf {
val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
- val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
+ val SERVER_BASE_PATH = Entry("livy.ui.basePath", "/")
Review comment:
I think I found it: `LivyConf.deprecatedConfigs`. I will add
`livy.ui.basePath` there, pointing it to the key `livy.server.basepath` instead.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309976585
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
Alright, I will remove it from the template and set the default to empty
string.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309975450
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
+ require(contextPath.startsWith("/"),
+s"Configuration property ${LivyConf.SERVER_BASE_PATH.key} must start with
/, e.g. /my-livy")
+ context.setContextPath(contextPath)
Review comment:
I believe those are correct as is. When testing the changes made things
seemed to work correctly. The redirect seems to function as expected, I see a
302 if I access the basePath without the ui part, with the browser redirecting
correctly. All the static resources, version, metrics, and API are accessible
as expected (no 404s seen when using the browser developer tools).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] yantzu opened a new pull request #193: [LIVY-621]add dynamic service discovry for thrift server
yantzu opened a new pull request #193: [LIVY-621]add dynamic service discovry for thrift server URL: https://github.com/apache/incubator-livy/pull/193 ## What changes were proposed in this pull request? Add config and implementation to allow publish livy thrift server to zookeeper. Configuration information are consistent with hiveserver2 for convenience. Publish information is the same as hiveserver2 has, so beeline can discover thrift server. https://issues.apache.org/jira/browse/LIVY-621 ## How was this patch tested? add unit test. And have tested manually. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309954905
##
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##
@@ -60,7 +60,7 @@ object LivyConf {
val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
- val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
+ val SERVER_BASE_PATH = Entry("livy.ui.basePath", "/")
Review comment:
It's been a while since I wrote it, but IIRC there's a section of code to
put old config names in and what new config to point to, we made it for
deprecating configs.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309954632
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
that makes sense and my bad, I had suggested to change `livy.ui.basePath =`
to `livy.ui.basePath = ""` in the previous PR not realizing what it'd do. But
since this is an optional config maybe it'd be better to just remove it from
the template and leave the default in the code as `""`, I would argue leaving
it unset is better than setting it to the default in a config.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] gumartinm opened a new pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client
gumartinm opened a new pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client URL: https://github.com/apache/incubator-livy/pull/192 ## What changes were proposed in this pull request? Batch session should end with state killed when process exits with no 0 return code. https://issues.apache.org/jira/browse/LIVY-620 ## How was this patch tested? Unit Test (included in this PR) Submit batch session that runs forever, wait 2 seconds, kill that batch session and expect for state killed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] gumartinm closed pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client
gumartinm closed pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client URL: https://github.com/apache/incubator-livy/pull/192 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309953184
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
+ require(contextPath.startsWith("/"),
Review comment:
You are correct, it should be removed.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309953116
##
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##
@@ -60,7 +60,7 @@ object LivyConf {
val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
- val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
+ val SERVER_BASE_PATH = Entry("livy.ui.basePath", "/")
Review comment:
I agree, it will be much clearer to the user what the intent is. The old
property can simply reference the new `livy.server.basePath` property to make
it backwards compatible.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309952626
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
You are right that the default could actually stay an empty string, but
after some pondering I changed the default for two reasons:
1. The original double quote in the template to indicate an empty string
(i.e. "") actually causes the contextPath to be come
http://localhost:8998/""/ui when enabled (this at least when I tested it). So
the template should actually be `livy.ui.basePath = ` but by changing it I
figured I might as well use `livy.ui.basePath = /` because
2. the basePath *must* start with a /. I figured by having the default a /
it would give a hint to the user that this is the minimum this property must
have.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] fdeantoni commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309952626
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
You are right that the default could actually stay an empty string, but
after some pondering I changed the default for two reasons:
# The original double quote in the template to indicate an empty string
(i.e. "") actually causes the contextPath to be come
http://localhost:8998/""/ui when enabled (this at least when I tested it). So
the template should actually be `livy.ui.basePath = ` but by changing it I
figured I might as well use `livy.ui.basePath = /` because
# the basePath *must* start with a /. I figured by having the default a / it
would give a hint to the user that this is the minimum this property must have.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] arunmahadevan commented on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes
arunmahadevan commented on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes URL: https://github.com/apache/incubator-livy/pull/167#issuecomment-517510038 @jahstreet , looks like if the livy pod restarts, the application link (that points to driverUI) for the interactive session does not appear anymore. The log link also doesn't seem to work. Wont Livy poll k8s to update the driver UI url or is it something broken in the Livy session recovery? This works for the batch session though. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] gumartinm opened a new pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client
gumartinm opened a new pull request #192: [LIVY-620] Spark batch session always ends with success when configuration is master yarn and deploy-mode client URL: https://github.com/apache/incubator-livy/pull/192 ## What changes were proposed in this pull request? Batch session should end with state killed when process exits with no 0 return code. https://issues.apache.org/jira/browse/LIVY-620 ## How was this patch tested? Unit Test (included in this PR) Submit batch session that runs forever, wait 2 seconds, kill that batch session and expect for state killed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on issue #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on issue #188: LIVY-615: Set context path using property SERVER_BASE_PATH. URL: https://github.com/apache/incubator-livy/pull/188#issuecomment-517458899 Also @m-wcislo would you be willing to read through our discussion here and provide some insight? You originally added the feature being discussed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309891166
##
File path: server/src/main/scala/org/apache/livy/LivyConf.scala
##
@@ -60,7 +60,7 @@ object LivyConf {
val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
- val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
+ val SERVER_BASE_PATH = Entry("livy.ui.basePath", "/")
Review comment:
Based on the change in use case and original intent I believe we should
change this to `livy.server.basePath` (and since it was released already add a
back-compatibility handler)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309896889
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
+ require(contextPath.startsWith("/"),
+s"Configuration property ${LivyConf.SERVER_BASE_PATH.key} must start with
/, e.g. /my-livy")
+ context.setContextPath(contextPath)
Review comment:
with the change to context here do the ui related `mounts` below still work
as expected? Especially the `uiRedirectServlets` or do they need to be updated?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309888447
##
File path: server/src/main/scala/org/apache/livy/server/LivyServer.scala
##
@@ -67,7 +67,9 @@ class LivyServer extends Logging {
val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
-val basePath = livyConf.get(SERVER_BASE_PATH)
+val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")
Review comment:
Based on the code you wrote here you should be able to leave the default
value in `livy.conf.template` and `LivyConf.scala` as `""` rather than `/`
since you immediately remove the trailing `/` here.
But nice catch about the trailing `/`, removing that is key to the js code
functioning correctly and would have been a mis-config triggered bug
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309892553
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
+ require(contextPath.startsWith("/"),
Review comment:
this `require` will always pass since `/` is always added to `contextPath`
above, you should remove it.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309886266
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
Review comment:
As a follow up I read through the original PR and both this and its JIRAs to
trace the intention, implementation, conversations, etc. Based on the original
PR conversation I believe this PR does what the original author intended. I
believe @jerryshao and myself misunderstood the author's intention previously
and thus missed the necessity of this extra bit of code. With this in mind I'll
do a proper code review of this PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] ajbozarth commented on a change in pull request #188: LIVY-615: Set context path using property SERVER_BASE_PATH.
ajbozarth commented on a change in pull request #188: LIVY-615: Set context
path using property SERVER_BASE_PATH.
URL: https://github.com/apache/incubator-livy/pull/188#discussion_r309885071
##
File path: server/src/main/scala/org/apache/livy/server/WebServer.scala
##
@@ -81,7 +81,10 @@ class WebServer(livyConf: LivyConf, var host: String, var
port: Int) extends Log
val context = new ServletContextHandler()
- context.setContextPath("/")
+ private val contextPath = "/" +
livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")
Review comment:
Ok so I think I'm up to speed, but just to make sure. You're saying that the
reverse proxy featured we added a while ago just got released in 0.6 but is
broken and doesn't work (I would assume normal non-reverse proxy works fine).
The problem being that the implementation meant to only tinker with the UI but
needed to affect the whole server instead. This PR is a fix that tweaks the
reverse proxy config to affect the whole server rather than the UI only. This
would bring the usage in line with how the code was written rather that how it
was intended to work. You chose this option because bringing the code in line
with its intention would require more and potentially messy changes as you
explained. IIUC I believe this PR is the correct course of action and if merged
would warrant a patch release.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-livy] o-shevchenko edited a comment on issue #189: [LIVY-616] Livy Server discovery
o-shevchenko edited a comment on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517356964 I have added API for getting Livy Server address to LivyClient, HttpClient, RSCClient and LivyScalaClient. I don't see a way to forward Livy address from DiscoveryManager to HttpClient or LivyScalaClient (we need to add a dependency on `livy-server` to `client-http` in this case, I'm not sure that this is acceptable). So, from client code we can use: ``` val address = DiscoveryManager.getServerUri() - here we can use it in REST requests val client = LivyClientBuilder.setURI(s"http://$address;).build - or create client val futureUri = client.getServerUri - and get address from client if needed val scalaClient = new LivyScalaClient(client) val futureUri = scalaClient.getServerUri ``` Added scaladocs, small refactoring. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery
o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517356964 I have added API for getting Livy Server address to LivyClient, HttpClient, RSCClient and LivyScalaClient. I don't see a way to forward Livy address from DiscoveryManager to HttpClient or LivyScalaClient (we need to add a dependency on `livy-server` to `client-http` in this case, I'm not sure that this is acceptable). So, from client code we can use: ``` val address = DiscoveryManager.getServerUri() - here we can use it in REST requests val client = LivyClientBuilder.setURI(s"http://address;).build - or create client val futureUri = client.getServerUri - and get address from client if needed val scalaClient = new LivyScalaClient(client) val futureUri = scalaClient.getServerUri ``` Added scaladocs, small refactoring. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes
codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes URL: https://github.com/apache/incubator-livy/pull/167#issuecomment-481824707 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=h1) Report > Merging [#167](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/7dee3cc142f79a131d54a37c7a56bb697c160cc1?src=pr=desc) will **decrease** coverage by `4.45%`. > The diff coverage is `8.55%`. [](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #167 +/- ## - Coverage 68.6% 64.14% -4.46% - Complexity 904 910 +6 Files 100 101 +1 Lines 5666 6114 +448 Branches850 908 +58 + Hits 3887 3922 +35 - Misses 1225 1631 +406 - Partials554 561 +7 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...e/livy/server/interactive/InteractiveSession.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvaW50ZXJhY3RpdmUvSW50ZXJhY3RpdmVTZXNzaW9uLnNjYWxh) | `68.9% <ø> (-0.22%)` | `44 <0> (ø)` | | | [...ala/org/apache/livy/utils/SparkKubernetesApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0t1YmVybmV0ZXNBcHAuc2NhbGE=) | `0% <0%> (ø)` | `0 <0> (?)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.57% <0%> (+0.6%)` | `42 <0> (+1)` | :arrow_up: | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.36% <100%> (+0.5%)` | `22 <1> (+1)` | :arrow_up: | | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.46% <20%> (-1.5%)` | `11 <1> (ø)` | | | [...rc/main/scala/org/apache/livy/utils/SparkApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0FwcC5zY2FsYQ==) | `67.5% <55.55%> (-8.5%)` | `1 <0> (ø)` | | | [...in/scala/org/apache/livy/repl/SQLInterpreter.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9TUUxJbnRlcnByZXRlci5zY2FsYQ==) | `62.5% <0%> (-7.88%)` | `9% <0%> (+2%)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | | | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `71.12% <0%> (-2.12%)` | `33% <0%> (ø)` | | | [...ala/org/apache/livy/utils/LineBufferedStream.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaW5lQnVmZmVyZWRTdHJlYW0uc2NhbGE=) | `80.48% <0%> (-1.1%)` | `5% <0%> (ø)` | | | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=footer). Last update [7dee3cc...abab55e](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[GitHub] [incubator-livy] codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes
codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes URL: https://github.com/apache/incubator-livy/pull/167#issuecomment-481824707 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=h1) Report > Merging [#167](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/7dee3cc142f79a131d54a37c7a56bb697c160cc1?src=pr=desc) will **decrease** coverage by `4.45%`. > The diff coverage is `8.55%`. [](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #167 +/- ## - Coverage 68.6% 64.14% -4.46% - Complexity 904 910 +6 Files 100 101 +1 Lines 5666 6114 +448 Branches850 908 +58 + Hits 3887 3922 +35 - Misses 1225 1631 +406 - Partials554 561 +7 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...e/livy/server/interactive/InteractiveSession.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvaW50ZXJhY3RpdmUvSW50ZXJhY3RpdmVTZXNzaW9uLnNjYWxh) | `68.9% <ø> (-0.22%)` | `44 <0> (ø)` | | | [...ala/org/apache/livy/utils/SparkKubernetesApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0t1YmVybmV0ZXNBcHAuc2NhbGE=) | `0% <0%> (ø)` | `0 <0> (?)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.57% <0%> (+0.6%)` | `42 <0> (+1)` | :arrow_up: | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.36% <100%> (+0.5%)` | `22 <1> (+1)` | :arrow_up: | | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.46% <20%> (-1.5%)` | `11 <1> (ø)` | | | [...rc/main/scala/org/apache/livy/utils/SparkApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0FwcC5zY2FsYQ==) | `67.5% <55.55%> (-8.5%)` | `1 <0> (ø)` | | | [...in/scala/org/apache/livy/repl/SQLInterpreter.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9TUUxJbnRlcnByZXRlci5zY2FsYQ==) | `62.5% <0%> (-7.88%)` | `9% <0%> (+2%)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | | | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `71.12% <0%> (-2.12%)` | `33% <0%> (ø)` | | | [...ala/org/apache/livy/utils/LineBufferedStream.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaW5lQnVmZmVyZWRTdHJlYW0uc2NhbGE=) | `80.48% <0%> (-1.1%)` | `5% <0%> (ø)` | | | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=footer). Last update [7dee3cc...abab55e](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[GitHub] [incubator-livy] codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes
codecov-io edited a comment on issue #167: [LIVY-588][WIP]: Full support for Spark on Kubernetes URL: https://github.com/apache/incubator-livy/pull/167#issuecomment-481824707 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=h1) Report > Merging [#167](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/7dee3cc142f79a131d54a37c7a56bb697c160cc1?src=pr=desc) will **decrease** coverage by `4.45%`. > The diff coverage is `8.55%`. [](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #167 +/- ## - Coverage 68.6% 64.14% -4.46% - Complexity 904 910 +6 Files 100 101 +1 Lines 5666 6114 +448 Branches850 908 +58 + Hits 3887 3922 +35 - Misses 1225 1631 +406 - Partials554 561 +7 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...e/livy/server/interactive/InteractiveSession.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvaW50ZXJhY3RpdmUvSW50ZXJhY3RpdmVTZXNzaW9uLnNjYWxh) | `68.9% <ø> (-0.22%)` | `44 <0> (ø)` | | | [...ala/org/apache/livy/utils/SparkKubernetesApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0t1YmVybmV0ZXNBcHAuc2NhbGE=) | `0% <0%> (ø)` | `0 <0> (?)` | | | [...ain/java/org/apache/livy/rsc/driver/RSCDriver.java](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvUlNDRHJpdmVyLmphdmE=) | `78.57% <0%> (+0.6%)` | `42 <0> (+1)` | :arrow_up: | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `96.36% <100%> (+0.5%)` | `22 <1> (+1)` | :arrow_up: | | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `34.46% <20%> (-1.5%)` | `11 <1> (ø)` | | | [...rc/main/scala/org/apache/livy/utils/SparkApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya0FwcC5zY2FsYQ==) | `67.5% <55.55%> (-8.5%)` | `1 <0> (ø)` | | | [...in/scala/org/apache/livy/repl/SQLInterpreter.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-cmVwbC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2xpdnkvcmVwbC9TUUxJbnRlcnByZXRlci5zY2FsYQ==) | `62.5% <0%> (-7.88%)` | `9% <0%> (+2%)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `52.94% <0%> (-2.95%)` | `7% <0%> (ø)` | | | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `71.12% <0%> (-2.12%)` | `33% <0%> (ø)` | | | [...ala/org/apache/livy/utils/LineBufferedStream.scala](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9MaW5lQnVmZmVyZWRTdHJlYW0uc2NhbGE=) | `80.48% <0%> (-1.1%)` | `5% <0%> (ø)` | | | ... and [4 more](https://codecov.io/gh/apache/incubator-livy/pull/167/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=footer). Last update [7dee3cc...abab55e](https://codecov.io/gh/apache/incubator-livy/pull/167?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[GitHub] [incubator-livy] jerryshao closed pull request #191: [MINOR] Remove unused guava import
jerryshao closed pull request #191: [MINOR] Remove unused guava import URL: https://github.com/apache/incubator-livy/pull/191 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] codecov-io edited a comment on issue #189: [LIVY-616] Livy Server discovery
codecov-io edited a comment on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517236826 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=h1) Report > Merging [#189](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/1ce266d78fb3475af6154262629ff92f5d342ae6?src=pr=desc) will **decrease** coverage by `0.16%`. > The diff coverage is `81.57%`. [](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #189 +/- ## - Coverage 68.71% 68.54% -0.17% - Complexity 908 920 +12 Files 100 103 +3 Lines 5674 5704 +30 Branches854 853 -1 + Hits 3899 3910 +11 - Misses 1219 1236 +17 - Partials556 558 +2 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `78.94% <ø> (-4.39%)` | `1 <0> (ø)` | | | [.../org/apache/livy/server/discovery/JsonMapper.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L0pzb25NYXBwZXIuc2NhbGE=) | `100% <100%> (ø)` | `0 <0> (?)` | | | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `37.91% <100%> (+2.93%)` | `15 <4> (+4)` | :arrow_up: | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.87% <100%> (+0.02%)` | `21 <0> (ø)` | :arrow_down: | | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `66.66% <71.42%> (-12.41%)` | `6 <5> (-11)` | | | [...pache/livy/server/discovery/DiscoveryManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L0Rpc2NvdmVyeU1hbmFnZXIuc2NhbGE=) | `75% <75%> (ø)` | `3 <3> (?)` | | | [...pache/livy/server/discovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L1pvb0tlZXBlck1hbmFnZXIuc2NhbGE=) | `78.72% <78.72%> (ø)` | `18 <18> (?)` | | | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `73.23% <0%> (-6.34%)` | `33% <0%> (ø)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `50% <0%> (-5.89%)` | `7% <0%> (ø)` | | | [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | | | ... and [7 more](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=footer). Last update [1ce266d...9731aae](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[GitHub] [incubator-livy] codecov-io commented on issue #189: [LIVY-616] Livy Server discovery
codecov-io commented on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517236826 # [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=h1) Report > Merging [#189](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=desc) into [master](https://codecov.io/gh/apache/incubator-livy/commit/1ce266d78fb3475af6154262629ff92f5d342ae6?src=pr=desc) will **decrease** coverage by `0.16%`. > The diff coverage is `81.57%`. [](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=tree) ```diff @@ Coverage Diff @@ ## master #189 +/- ## - Coverage 68.71% 68.54% -0.17% - Complexity 908 920 +12 Files 100 103 +3 Lines 5674 5704 +30 Branches854 853 -1 + Hits 3899 3910 +11 - Misses 1219 1236 +17 - Partials556 558 +2 ``` | [Impacted Files](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=tree) | Coverage Δ | Complexity Δ | | |---|---|---|---| | [...a/org/apache/livy/server/recovery/StateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvU3RhdGVTdG9yZS5zY2FsYQ==) | `78.94% <ø> (-4.39%)` | `1 <0> (ø)` | | | [.../org/apache/livy/server/discovery/JsonMapper.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L0pzb25NYXBwZXIuc2NhbGE=) | `100% <100%> (ø)` | `0 <0> (?)` | | | [...main/scala/org/apache/livy/server/LivyServer.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvTGl2eVNlcnZlci5zY2FsYQ==) | `37.91% <100%> (+2.93%)` | `15 <4> (+4)` | :arrow_up: | | [...rver/src/main/scala/org/apache/livy/LivyConf.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9MaXZ5Q29uZi5zY2FsYQ==) | `95.87% <100%> (+0.02%)` | `21 <0> (ø)` | :arrow_down: | | [...che/livy/server/recovery/ZooKeeperStateStore.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvcmVjb3ZlcnkvWm9vS2VlcGVyU3RhdGVTdG9yZS5zY2FsYQ==) | `66.66% <71.42%> (-12.41%)` | `6 <5> (-11)` | | | [...pache/livy/server/discovery/DiscoveryManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L0Rpc2NvdmVyeU1hbmFnZXIuc2NhbGE=) | `75% <75%> (ø)` | `3 <3> (?)` | | | [...pache/livy/server/discovery/ZooKeeperManager.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zZXJ2ZXIvZGlzY292ZXJ5L1pvb0tlZXBlck1hbmFnZXIuc2NhbGE=) | `78.72% <78.72%> (ø)` | `18 <18> (?)` | | | [...ain/scala/org/apache/livy/utils/SparkYarnApp.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2VydmVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS91dGlscy9TcGFya1lhcm5BcHAuc2NhbGE=) | `73.23% <0%> (-6.34%)` | `33% <0%> (ø)` | | | [...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-c2NhbGEtYXBpL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvbGl2eS9zY2FsYWFwaS9TY2FsYUpvYkhhbmRsZS5zY2FsYQ==) | `50% <0%> (-5.89%)` | `7% <0%> (ø)` | | | [...in/java/org/apache/livy/rsc/driver/JobWrapper.java](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree#diff-cnNjL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9saXZ5L3JzYy9kcml2ZXIvSm9iV3JhcHBlci5qYXZh) | `82.85% <0%> (-5.72%)` | `8% <0%> (-1%)` | | | ... and [7 more](https://codecov.io/gh/apache/incubator-livy/pull/189/diff?src=pr=tree-more) | | -- [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=continue). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=footer). Last update [1ce266d...9731aae](https://codecov.io/gh/apache/incubator-livy/pull/189?src=pr=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
[GitHub] [incubator-livy] o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery
o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517236195 BinaryThriftServerSuite."fetch different data types" looks flaky https://travis-ci.org/apache/incubator-livy/jobs/566398488 ``` BinaryThriftServerSuite: 2328- Reuse existing session (30 seconds, 746 milliseconds) 2329- fetch different data types *** FAILED *** (4 seconds, 270 milliseconds) 2330 java.sql.SQLException: java.util.concurrent.ExecutionException: java.io.IOException: RSCClient instance stopped. 2331 at org.apache.hive.jdbc.HiveStatement.waitForOperationToComplete(HiveStatement.java:401) 2332 at org.apache.hive.jdbc.HiveStatement.execute(HiveStatement.java:266) 2333 at org.apache.hive.jdbc.HiveStatement.executeQuery(HiveStatement.java:497) 2334 at org.apache.livy.thriftserver.CommonThriftTests$class.dataTypesTest(ThriftServerSuites.scala:31) 2335 at org.apache.livy.thriftserver.BinaryThriftServerSuite.dataTypesTest(ThriftServerSuites.scala:74) 2336 at org.apache.livy.thriftserver.BinaryThriftServerSuite$$anonfun$2$$anonfun$apply$mcV$sp$2.apply(ThriftServerSuites.scala:98) 2337 at org.apache.livy.thriftserver.BinaryThriftServerSuite$$anonfun$2$$anonfun$apply$mcV$sp$2.apply(ThriftServerSuites.scala:97) 2338 at org.apache.livy.thriftserver.ThriftServerBaseTest$$anonfun$withJdbcStatement$1.apply(ThriftServerBaseTest.scala:102) 2339 at org.apache.livy.thriftserver.ThriftServerBaseTest$$anonfun$withJdbcStatement$1.apply(ThriftServerBaseTest.scala:99) 2340 at org.apache.livy.thriftserver.ThriftServerBaseTest.withJdbcConnection(ThriftServerBaseTest.scala:92) ``` Created separated ticket https://issues.apache.org/jira/browse/LIVY-618 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao commented on issue #191: [MINOR] Remove unused guava import
jerryshao commented on issue #191: [MINOR] Remove unused guava import URL: https://github.com/apache/incubator-livy/pull/191#issuecomment-517204487 CC @runzhiwang This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] o-shevchenko edited a comment on issue #189: [LIVY-616] Livy Server discovery
o-shevchenko edited a comment on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517193430 What IDEA plugin do you use for import ordering? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery
o-shevchenko commented on issue #189: [LIVY-616] Livy Server discovery URL: https://github.com/apache/incubator-livy/pull/189#issuecomment-517193430 What plugin do you use for import ordering? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [incubator-livy] jerryshao opened a new pull request #191: [MINOR] Remove unused guava import
jerryshao opened a new pull request #191: [MINOR] Remove unused guava import URL: https://github.com/apache/incubator-livy/pull/191 ## What changes were proposed in this pull request? PR #181 removed guava dependency in LivyServer, but it still left unused guava import. Here in this minor fix, removed this unused import. ## How was this patch tested? Existing UTs. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
