[GitHub] [incubator-livy] runzhiwang commented on a change in pull request #196: [LIVY-606][LIVY-152] fix the invalid conf of pyspark.archives and sparkr.package
runzhiwang commented on a change in pull request #196: [LIVY-606][LIVY-152] fix
the invalid conf of pyspark.archives and sparkr.package
URL: https://github.com/apache/incubator-livy/pull/196#discussion_r314978283
##
File path:
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSession.scala
##
@@ -247,30 +247,26 @@ object InteractiveSession extends Logging {
}
def findPySparkArchives(): Seq[String] = {
- Option(livyConf.get(RSCConf.Entry.PYSPARK_ARCHIVES))
Review comment:
@yiheng I have updated the pr. Please review it again. Thanks.
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] runzhiwang closed pull request #196: [LIVY-606][LIVY-152] fix the invalid conf of pyspark.archives and sparkr.package
runzhiwang closed pull request #196: [LIVY-606][LIVY-152] fix the invalid conf of pyspark.archives and sparkr.package URL: https://github.com/apache/incubator-livy/pull/196 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] yiheng commented on a change in pull request #201: [LIVY-639] add start time and completion time and duration to the statements web ui
yiheng commented on a change in pull request #201: [LIVY-639] add start time
and completion time and duration to the statements web ui
URL: https://github.com/apache/incubator-livy/pull/201#discussion_r315024386
##
File path: repl/src/main/scala/org/apache/livy/repl/Session.scala
##
@@ -147,6 +149,12 @@ class Session(
_statements.toMap
}
+ def nowTime(): String = {
+import java.time.format.DateTimeFormatter
+val formatter = DateTimeFormatter.ofPattern("-MM-dd HH:mm:ss")
+return LocalDateTime.now().format(formatter)
Review comment:
How do we handle the timezone? What is the timezone of the returned time
representation?
My concern is if the server and web client are in different timezones, will
there be some confusion?
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 a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
jerryshao commented on a change in pull request #197: [LIVY-574][THRIFT
SERVER][TEST] Add tests for metadata operations
URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315025250
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/MetadataOperation.scala
##
@@ -42,8 +46,14 @@ abstract class MetadataOperation(sessionHandle:
SessionHandle, opType: Operation
assertState(Seq(OperationState.FINISHED))
validateFetchOrientation(orientation)
if (orientation.equals(FetchOrientation.FETCH_FIRST)) {
- rowSet.setRowOffset(0)
+ offset = 0
+}
+
+if (offset >= rowSet.numRows) {
Review comment:
What's the pros and cons of these two methods?
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 a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
jerryshao commented on a change in pull request #197: [LIVY-574][THRIFT
SERVER][TEST] Add tests for metadata operations
URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315031069
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/MetadataOperation.scala
##
@@ -42,8 +46,14 @@ abstract class MetadataOperation(sessionHandle:
SessionHandle, opType: Operation
assertState(Seq(OperationState.FINISHED))
validateFetchOrientation(orientation)
if (orientation.equals(FetchOrientation.FETCH_FIRST)) {
- rowSet.setRowOffset(0)
+ offset = 0
+}
+
+if (offset >= rowSet.numRows) {
Review comment:
I see, thanks! IMO I think current solution is OK. Maybe we need to add some
comments on the code.
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] yiheng commented on a change in pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
yiheng commented on a change in pull request #200: [LIVY-637]Fix
NullPointerException when create database using thrifts…
URL: https://github.com/apache/incubator-livy/pull/200#discussion_r315044002
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/serde/ThriftResultSet.scala
##
@@ -112,6 +112,7 @@ class ColumnOrientedResultSet(
override def toTRowSet: TRowSet = {
val tRowSet = new TRowSet(rowOffset, new util.ArrayList[TRow])
+tRowSet.setColumns(new util.ArrayList[TColumn]())
Review comment:
should we add a comment here to explain why we have this?
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] yiheng commented on a change in pull request #196: [LIVY-606][LIVY-152] fix the invalid conf of pyspark.archives and sparkr.package
yiheng commented on a change in pull request #196: [LIVY-606][LIVY-152] fix the
invalid conf of pyspark.archives and sparkr.package
URL: https://github.com/apache/incubator-livy/pull/196#discussion_r315023671
##
File path:
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSession.scala
##
@@ -183,7 +183,7 @@ object InteractiveSession extends Logging {
}
def findSparkRArchive(): Option[String] = {
- Option(livyConf.get(RSCConf.Entry.SPARKR_PACKAGE.key())).orElse {
Review comment:
ok.
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] shanyu opened a new pull request #187: LIVY-617: Livy session leak on Yarn when creating session duplicated names
shanyu opened a new pull request #187: LIVY-617: Livy session leak on Yarn when creating session duplicated names URL: https://github.com/apache/incubator-livy/pull/187 ## What changes were proposed in this pull request? When creating a session with duplicated name, instead of throw exception in SessionManager.register() method, we should stop the session. Otherwise the session driver process will keep running and end up creating a leaked Yarn application. https://issues.apache.org/jira/browse/LIVY-617 ## How was this patch tested? This is just a simple fix and verified with manual end to end test. 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 closed pull request #187: LIVY-617: Livy session leak on Yarn when creating session duplicated names
jerryshao closed pull request #187: LIVY-617: Livy session leak on Yarn when creating session duplicated names URL: https://github.com/apache/incubator-livy/pull/187 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] captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 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] captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 ## What changes were proposed in this pull request? Spark beeline use old hive-jdbc-client doesn’t do null point ref check. So when new TRowSet, setColumes make sure column set not null. ## How was this patch tested? Connect to livy's thriftserver using spark beeline. And create/use/drop database will no longer have nullpointerexceptions after execution.  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] runzhiwang commented on a change in pull request #196: [LIVY-606][LIVY-152] fix the invalid conf of pyspark.archives and sparkr.package
runzhiwang commented on a change in pull request #196: [LIVY-606][LIVY-152] fix
the invalid conf of pyspark.archives and sparkr.package
URL: https://github.com/apache/incubator-livy/pull/196#discussion_r315023241
##
File path:
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSession.scala
##
@@ -183,7 +183,7 @@ object InteractiveSession extends Logging {
}
def findSparkRArchive(): Option[String] = {
- Option(livyConf.get(RSCConf.Entry.SPARKR_PACKAGE.key())).orElse {
Review comment:
@yiheng I read the SPARKR_PACKAGE from livy-client.conf instead of
livy.conf. The builderProperties has the config of livy-client.conf.
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] captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 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] captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 ## What changes were proposed in this pull request? Spark beeline use old hive-jdbc-client doesn’t do null point ref check. So when new TRowSet, setColumes make sure column set not null. ## How was this patch tested? Connect to livy's thriftserver using spark beeline. And create/use/drop database will no longer have nullpointerexceptions after execution.  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] yiheng commented on issue #199: [LIVY-573] Add tests for operation logs retrieval
yiheng commented on issue #199: [LIVY-573] Add tests for operation logs retrieval URL: https://github.com/apache/incubator-livy/pull/199#issuecomment-522400168 resolved 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 a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
jerryshao commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315023773 ## File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/GetTypeInfoOperation.scala ## @@ -44,21 +44,22 @@ class GetTypeInfoOperation(sessionHandle: SessionHandle) val data = Array[Any]( t.name, t.sqlType, - t.precision, + t.precision.getOrElse(null), Review comment: `orNull`. 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 #199: [LIVY-573] Add tests for operation logs retrieval
jerryshao commented on issue #199: [LIVY-573] Add tests for operation logs retrieval URL: https://github.com/apache/incubator-livy/pull/199#issuecomment-522396587 Please rebase the code. 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] captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 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] yiheng commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
yiheng commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315030085 ## File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/GetTypeInfoOperation.scala ## @@ -44,21 +44,22 @@ class GetTypeInfoOperation(sessionHandle: SessionHandle) val data = Array[Any]( t.name, t.sqlType, - t.precision, + t.precision.getOrElse(null), Review comment: fixed 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 a change in pull request #201: [LIVY-639] add start time and completion time and duration to the statements web ui
jerryshao commented on a change in pull request #201: [LIVY-639] add start time and completion time and duration to the statements web ui URL: https://github.com/apache/incubator-livy/pull/201#discussion_r315030128 ## File path: rsc/src/main/java/org/apache/livy/rsc/driver/Statement.java ## @@ -28,6 +28,9 @@ @JsonRawValue public volatile String output; public double progress; + public String duration; + public String started; + public String completed; Review comment: It's not so efficient to use string to represent time internally, timestamp should be enough. 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] yiheng commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
yiheng commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315030098 ## File path: thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/GetTypeInfoOperation.scala ## @@ -44,21 +44,22 @@ class GetTypeInfoOperation(sessionHandle: SessionHandle) val data = Array[Any]( t.name, t.sqlType, - t.precision, + t.precision.getOrElse(null), null, // LITERAL_PREFIX null, // LITERAL_SUFFIX null, // CREATE_PARAMS - DatabaseMetaData.typeNullable, // All types are nullable + DatabaseMetaData.typeNullable.toShort, // All types are nullable t.caseSensitive, + t.searchable, t.unsignedAttribute, false, // FIXED_PREC_SCALE false, // AUTO_INCREMENT null, // LOCAL_TYPE_NAME - 0, // MINIMUM_SCALE - 0, // MAXIMUM_SCALE + 0.toShort, // MINIMUM_SCALE + 0.toShort, // MAXIMUM_SCALE null, // SQL_DATA_TYPE null, // SQL_DATETIME_SUB - t.numPrecRadix) + t.numPrecRadix.getOrElse(null)) Review comment: fixed 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 a change in pull request #201: [LIVY-639] add start time and completion time and duration to the statements web ui
jerryshao commented on a change in pull request #201: [LIVY-639] add start time
and completion time and duration to the statements web ui
URL: https://github.com/apache/incubator-livy/pull/201#discussion_r315030442
##
File path: rsc/src/main/java/org/apache/livy/rsc/driver/Statement.java
##
@@ -56,4 +59,26 @@ public void updateProgress(double p) {
this.progress = p;
}
}
+
+ public void setDuration(String d) {
+if (this.state.get().isOneOf(StatementState.Cancelled,
StatementState.Available)) {
+ this.duration = d;
+}
+ }
+
+ public String getStarted() {
+return started;
+ }
+
+ public void setStarted(String started) {
+this.started = started;
+ }
+
+ public String getCompleted() {
+return completed;
+ }
+
+ public void setCompleted(String completed) {
+this.completed = completed;
+ }
Review comment:
As all the fields are public, it is not necessary to write all these
setter/getters.
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] yiheng commented on a change in pull request #197: [LIVY-574][THRIFT SERVER][TEST] Add tests for metadata operations
yiheng commented on a change in pull request #197: [LIVY-574][THRIFT
SERVER][TEST] Add tests for metadata operations
URL: https://github.com/apache/incubator-livy/pull/197#discussion_r315027255
##
File path:
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/operation/MetadataOperation.scala
##
@@ -42,8 +46,14 @@ abstract class MetadataOperation(sessionHandle:
SessionHandle, opType: Operation
assertState(Seq(OperationState.FINISHED))
validateFetchOrientation(orientation)
if (orientation.equals(FetchOrientation.FETCH_FIRST)) {
- rowSet.setRowOffset(0)
+ offset = 0
+}
+
+if (offset >= rowSet.numRows) {
Review comment:
@jerryshao
1. Return an empty ResultSet when needed
**pros**: No data copy. It's efficient
**cons**: Not straight forward. And we need to maintain the `row offset` in
the `MetadataOperation`.
2. Add `extractSubset()` to `ThriftResultSet`
**pros**: Straight forward. Maintain the `row offset` in the `ResultSet`
itself. It's what spark thriftserver do.
**cons**: Data copy every time. Honestly, I think it's not a big issue as
the metadata is small. However, `ThriftResultSet` is also used in other
places, e.g. fetch table data. Not sure if it will be misused in the future.
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] captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc closed pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 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] captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts…
captainzmc opened a new pull request #200: [LIVY-637]Fix NullPointerException when create database using thrifts… URL: https://github.com/apache/incubator-livy/pull/200 ## What changes were proposed in this pull request? Spark beeline use old hive-jdbc-client doesn’t do null point ref check. So when new TRowSet, setColumes make sure column set not null. ## How was this patch tested? Connect to livy's thriftserver using spark beeline. And create/use/drop database will no longer have nullpointerexceptions after execution.  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] yiheng opened a new pull request #199: [LIVY-573] Add tests for operation logs retrieval
yiheng opened a new pull request #199: [LIVY-573] Add tests for operation logs retrieval URL: https://github.com/apache/incubator-livy/pull/199 ## What changes were proposed in this pull request? Add unit test for operation logs retrieval. ## How was this patch tested? New unit tests and existing unit test 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
