You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2018/06/28 07:38:27 UTC

[kudu-CR] WIP: spark: Expose socketReadTimeoutMs at the Spark level

Mike Percy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10839


Change subject: WIP: spark: Expose socketReadTimeoutMs at the Spark level
......................................................................

WIP: spark: Expose socketReadTimeoutMs at the Spark level

This patch exposes socketReadTimeoutMs in the KuduContext and the
DefaultSource.

Needs tests.

Change-Id: I0ab0ff0b242790caffb7e2848958148ffe547c4d
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
4 files changed, 49 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/10839/1
-- 
To view, visit http://gerrit.cloudera.org:8080/10839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0ab0ff0b242790caffb7e2848958148ffe547c4d
Gerrit-Change-Number: 10839
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>

[kudu-CR] spark: Expose socketReadTimeoutMs to Spark connector

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10839 )

Change subject: spark: Expose socketReadTimeoutMs to Spark connector
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala:

http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala@188
PS3, Line 188: socketReadTimeoutMs match {
             :     case Some(_) => context.socketReadTimeoutMs = socketReadTimeoutMs
             :     case None =>
             :   }
simplify this to just

    context.socketReadTimeoutMs = socketReadTimeoutMs


http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:

http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@91
PS3, Line 91:   var socketReadTimeoutMs: Option[Long] = None
'var' is wrong here -- if the caller gets the asyncClient, changes the read timeout and then gets the asyncClient again it will have the original value.  I think it'd be better to add this as a val ctor param with a default value, i.e:

class KuduContext(val kuduMaster: String,
sc: SparkContext,
sockedReadTimeoutMs: Option[Long] = None) extends Serializable {


http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@111
PS3, Line 111:   import kudu.KuduContext._
What is this for? I'd remove if possible, underscore imports are non-explicit in really tricky ways.


http://gerrit.cloudera.org:8080/#/c/10839/3/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@399
PS3, Line 399: @InterfaceAudience.Private
These aren't necessary since it's a private object.



-- 
To view, visit http://gerrit.cloudera.org:8080/10839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0ab0ff0b242790caffb7e2848958148ffe547c4d
Gerrit-Change-Number: 10839
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 11 Jul 2018 19:05:58 +0000
Gerrit-HasComments: Yes